Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

try to respect outside CXXFLAGS #402

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bgruening
Copy link

@chhylp123 can you maybe try that?

@chhylp123
Copy link
Owner

chhylp123 commented Feb 19, 2023

@bgruening I see. The problem is that the new Makefile clearly introduces the CXXFLAGS, which will utilize the CXXFLAGS environment variable of users. If the CXXFLAGS environment variable is set by bioconda, makefile will still be failed?

@chhylp123
Copy link
Owner

@bgruening Sorry I mean if users would like to compile hifiasm manually from the source code, they will simiply type make. In this case, makefile will introduce the CXXFLAGS environment variable.

@bgruening
Copy link
Author

Yes, you need to find a solution to set all the parameters when a user executes make but this should still respect the ENV vars set by the user externally - in this case, conda.

You could also use VARIABLE ?= value to just set CXXFLAGS when its not set outside.

@chhylp123
Copy link
Owner

@bgruening I see, thanks a lot. I will merge it in the next release. As I already cut a release for the 0.18.7, I just want to figure out a way to modify conda receipt, instead of the Makefile. This is also helpful if the users would like to utilize previous versions by bioconda. I'm thinking probably could I directly incorporate -L within CXXFLAGS (although it is not a good solution)?:

make CXXFLAGS="-g -I$PREFIX/include -O3 -msse4.2 -mpopcnt -fomit-frame-pointer -Wall -L$PREFIX/lib" CC=${CC} CXX=${CXX}

@bgruening
Copy link
Author

If you are searching for a hack, remove the lines from your Makefile as part of the conda recipe :)

See here for inspiration: https://github.com/bioconda/bioconda-recipes/blob/f7060664019d76f30ec726d1e3981d4ec2cdb336/recipes/vcflib/build.sh#L9

@chhylp123
Copy link
Owner

Thanks a lot! Let me update the the makefile and the receipt with the new release.

@bgruening
Copy link
Author

@chhylp123 can you ping me when this is released please? Thanks a lot!

@chhylp123
Copy link
Owner

@bgruening Sorry for the delay as I was working on some bugs inside hifiasm. As you mentioned on another issue (#392 (comment)):

This means zlib is not found and I guess this is because you are overwriting CXXFLAGS in your Makefile.

Yes, the GUN make has an override behavior (https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_9.html#SEC90), which means the 'CXXFLAGS' inside the makefile will be overrided by that in the command line. I understand this is why the previous conda version of hifiasm is several times slower as -O3 will be overrided by the conda receipt (https://github.com/bioconda/bioconda-recipes/blob/fca9554c95763983bdc9fb06770f956b90967057/recipes/hifiasm/build.sh#L5). I do understand your following modification for the makefile will be helpful to make hifiasm work with exisiting bioconda receipt. But my question is why the following updated conda receipt cannot pass the review of bioconda?

bioconda/bioconda-recipes@8b5b243

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

PS: there is an urgent bug to be fixed in hifiasm, so I will cut a new release first without your modification.

@bgruening
Copy link
Author

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

You are hardcoding these values in your Makefile and do not allow the overwrites. It has nothing to do with Conda or Bioconda you can reproduce this locally as well.

You need to define variables with ?= like here https://github.com/dpryan79/libBigWig/blob/master/Makefile#L4 or := depending on what you need.

@bgruening
Copy link
Author

I hacked your Makefile on the bioconda side. Could you maybe try version 0.18.8 and see if its faster?

@chhylp123
Copy link
Owner

chhylp123 commented Feb 23, 2023

@bgruening Thanks a lot. But as this issue mentioned (weidai11/cryptopp#525), if users set CXXFLAGS by the command line, the GNU make will overwrite CXXFLAGS no matter it is hardcoded in Makefile or not? With the hardcoded Makefile and the previous bioconda receipt (https://github.com/bioconda/bioconda-recipes/blob/fca9554c95763983bdc9fb06770f956b90967057/recipes/hifiasm/build.sh#L5), hifiasm works with the bioconda but loses -O3. Therefore I guess the GNU make will overwrite CXXFLAGS in anyway?

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

You are hardcoding these values in your Makefile and do not allow the overwrites. It has nothing to do with Conda or Bioconda you can reproduce this locally as well.

You need to define variables with ?= like here https://github.com/dpryan79/libBigWig/blob/master/Makefile#L4 or := depending on what you need.

@bgruening
Copy link
Author

@chhylp123 mh ... sorry, this week is just crazy. I can not really test it but I guess it would also work if you write:

make CXXFLAGS="-g -I$PREFIX/include -O3 -msse4.2 -mpopcnt -fomit-frame-pointer -Wall" LIBS="-L$PREFIX/lib -lz -lpthread -lm" CC=${CC} CXX=${CXX}

So using your LIBS variable.

@chhylp123
Copy link
Owner

@bgruening Thanks a lot! Have a nice weekend~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants