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

WIP: DO NOT MERGE: demonstration of $CONFIG_SITE #87

Closed
wants to merge 5 commits into from
Closed

WIP: DO NOT MERGE: demonstration of $CONFIG_SITE #87

wants to merge 5 commits into from

Conversation

gillins
Copy link
Contributor

@gillins gillins commented Jul 2, 2016

Do not merge.

This is a demonstration of using the site defaults feature of configure to get the right flags to the compiler without breaking libtool.

Xref: conda-forge/conda-forge.github.io#183, #85.

Background: toolchain sets CC, CFLAGS etc environment variables to control the flags passed to the compiler. This seems to break libtool. One option is to pass the variables on the command line like #85 but this is error prone. Another possibility is to have our own libtool, but this seems likely to be a lot of work.

autoconf has a “Site Defaults” feature (https://www.gnu.org/software/autoconf/manual/autoconf-2.68/html_node/Site-Defaults.html) and this PR demonstrates having a file with the flags and setting the $CONFIG_SITE environment variable to it. Ideally, this would be done by toolchain. None of the CC, CFLAGS environment variables need to be set.

One drawback of this approach is that cmake and other build systems won't have access to the environment variables – maybe they will need to source $CONFIG_SITE or something similar.

Apologies if this isn't clear. And sorry, some of my other changes seem to have ended up here too...

cc @kmuehlbauer @ocefpaf @jakirkham

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 2, 2016

@gillins I am OK with this and, if it works, I am fine merging it. But isn't it virtually the same thing as passing those variables (or some of those) in the build.sh directly?

PS: I am thinking about ditching the toolchain altogether and just copy what ever we need to make things more explicit and easy to stop.

@gillins
Copy link
Contributor Author

gillins commented Jul 2, 2016

Yes indeed it is. Just means it is done automatically without the recipe writer having to do anything...

If people are ok with idea, we should update toolchain instead so all feedstocks get it. So don't merge this one 😄

@gillins gillins changed the title WIP: demonstration of $CONFIG_SITE WIP: DO NOT MERGE: demonstration of $CONFIG_SITE Jul 2, 2016
@jakirkham
Copy link
Member

Thanks for trying this out, @gillins. This approach sounds very intriguing to me. If it works, I would be more than happy to see it included into the toolchain and reduce burden on packagers.

One drawback of this approach is that cmake and other build systems won't have access to the environment variables – maybe they will need to source $CONFIG_SITE or something similar.

So, I have a question on this point though. Is this needed for cmake?

IIRC we tried using the current toolchain with cmake before and it doesn't seem to have the same issues that libtool has. Also, I think using the workaround that you came up with causes issues for cmake. So, we may be better off not using the same solution.

@jakirkham
Copy link
Member

jakirkham commented Jul 2, 2016

PS: I am thinking about ditching the toolchain altogether and just copy what ever we need to make things more explicit and easy to stop.

Just FYI, @ocefpaf, part of the reason I helped created the toolchain was I was completely overwhelmed helping fixing these flags everywhere (when asked) and making sure we were following standard build practices. Clearly some packages slipped through the cracks, which is causing us problems now.

While I understand you are frustrated that it is not working smoothly (I am too and imagine others are as well), going away from standard build practices is unmaintainable IMHO and will get us into the same mess we are in now. If the standard build practices are not working, we should try to figure out how to fix the standard. I'm more than willing to be involved in the process of fixing the standard.

cc @pelson

@gillins
Copy link
Contributor Author

gillins commented Jul 3, 2016

Agreed that cmake does not have the same issues. It seems to be able to use the CFLAGS etc environment variables fine. But if we do not set them (and set $CONFIG_SITE instead for libtool) it won't get these flags. toolchain won't know if recipe if configure or cmake based...

Solution is maybe for recipes using cmake there has to be a line sourcing something to set $CFLAGS etc?

@gillins
Copy link
Contributor Author

gillins commented Jul 3, 2016

I have found that building with this recipe seems to fix the python-bindings-linked-to-build-location-of-gdal problem (conda-forge/fiona-feedstock#17 (comment)). Very strange, but maybe setting the $LDFLAGS env var or something like that was breaking it?

Unfortunately I'm going to be away for most of the coming week so won't be able to do much testing on OSX...

@ocefpaf
Copy link
Member

ocefpaf commented Jul 3, 2016

Unfortunately I'm going to be away for most of the coming week so won't be able to do much testing on OS X...

No problem. I will perform some tests. Also, I don't want to merge this until xerces-c is up to avoid another release.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 7, 2016

@pelson I re-started the CIs here to force it to get xerces-c from conda-forge instead of defaults.(xerces-c was added after this PR started.)

@pelson
Copy link
Member

pelson commented Jul 7, 2016

I'd prefer to be doing this as a file, rather than as a patch, but this to me looks like a really healthy approach. The question then is, do we duplicate this file in all feedstocks that need it (redundancy, risk of being inconsistent, but fully encapsulates the build context), or do we ship it with toolchain and put the .cfg in place as part of the GDAL build script itself?

@gillins
Copy link
Contributor Author

gillins commented Jul 8, 2016

Looks like something is linked to /usr/lib/libiconv which is a bit strange. Hopefully will be able to look at this next week.

The idea of this PR was to demonstrate $CONFIG_SITE and maybe make the toolchain based on that if it makes sense. The config.site file would then be part of toolchain. I hadn't really thought of making it a gdal only thing. I suspect if gdal needs it, then there will be other packages that do also.

That is if it is decided to continue with the toolchain...

@ocefpaf
Copy link
Member

ocefpaf commented Jul 8, 2016

Looks like something is linked to /usr/lib/libiconv which is a bit strange.

I am trying to figure out what changed and I will report here any findinds.

That is if it is decided to continue with the toolchain...

I am 👎 to the toolchain. I'd rather have any necessary flags hardcoded in the build.sh. Or, if we fix the openblas/Fortran issue, no flag at all like we had before.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 17, 2016

@gillins since I merged #90 are you OK closing this?

@gillins
Copy link
Contributor Author

gillins commented Jul 17, 2016

Absolutely - thanks for all your hard work on this @ocefpaf and @kmuehlbauer !

@gillins gillins closed this Jul 17, 2016
@ocefpaf
Copy link
Member

ocefpaf commented Jul 17, 2016

Absolutely - thanks for all your hard work on this @ocefpaf and @kmuehlbauer !

And you! It took 3 heads to crack this one 😉

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.

5 participants