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

Libiconv in build inputs leads to subtly broken cflags on Linux #19895

Closed
shlevy opened this issue Oct 26, 2016 · 12 comments
Closed

Libiconv in build inputs leads to subtly broken cflags on Linux #19895

shlevy opened this issue Oct 26, 2016 · 12 comments

Comments

@shlevy
Copy link
Member

shlevy commented Oct 26, 2016

glibc includes iconv functions so we used to have libiconvOrNull to avoid a spurious build input on systems that have glibc as their libc. We removed this in #6166 (for good reason, IMO) and now just have libiconv as an alias for glibc on those systems. Unfortunately, including glibc as a build input puts its include headers on the search path with isystem instead of just using the idirafter baked into the cc wrapper, which means glibc's headers are no longer searched last in the list as desired. This leads to the bug referenced in #19834 (see discussion on #19859 ), and in general will cause problems for anything that builds with libc++, depends on libiconv, and references <cmath> on glibc systems.

I'm not sure the best solution here. We could ignore instances of the c library our compiler is built with in build inputs, we could have some logic in glibc's setup hook to tell it to add idirafter instead of isystem, we could bring back libiconvOrNull or something like it... None of these seem obviously good or obviously awful. Thoughts?

@shlevy
Copy link
Member Author

shlevy commented Oct 26, 2016

@copumpkin
Copy link
Member

My fear with libiconvOrNull is that people might assume derivation references are non-null (e.g., map over a list of them and ask for .bin on them). My ideal path forward would be to make it "no big deal" to have glibc as a buildInput, but that mostly stems from the preferences I outlined in #6166

@gridaphobe
Copy link
Contributor

I'm inclined to side with @copumpkin here, I don't want to resurrect libiconvOrNull.

An alternative to @copumpkin's suggestion (I'm not sure how difficult that would be) would be to include libiconv in the Darwin stdenv as it (implicitly) is on Linux. I think I suggested this a while ago and there were objections, but it really does seem to be the simplest solution to me. More generally, I think we should specify a common set of libraries that stdenvs must supply. This might in practice be "anything glibc provides" but at least then we'd have feature parity across stdenvs.

@copumpkin
Copy link
Member

@gridaphobe from the old PR:

My basic stance is that I prefer precise dependencies, and don't want every stdenv for the rest of eternity to be bound by glibc's decisions to bundle a whole bunch of stuff. If we can get packages that depend on libiconv (and resolv, and others) to be explicit about that, that'll significantly facilitate bootstrapping other platforms that might also not bundle all the stuff glibc bundles.

@gridaphobe
Copy link
Contributor

We don't have to be bound by glibc's decisions though, which I think is what you're getting at in your 2nd paragraph:

Alternately we could just bite the bullet and make a "standard nix libc interface" and declare that any libc implementation needs to provide that functionality (and ideally no more). It'll make me a bit sad, but if it's an explicit decision it's not the end of the world.

We could say that a libc should provide certain things, excluding libiconv, and then hide the relevant symbols in glibc to make it compliant. I don't have a strong opinion on a large vs small standard libc, but it really does seem more sane to me than dealing with a complex chain of conditionals in each package that might be bundled with glibc.

@shlevy
Copy link
Member Author

shlevy commented Oct 26, 2016

OK, to make it "no big deal" to have glibc as a build input, we probably want some nix-support file that the cc-wrapper setup hook checks to disable adding the isystem etc. flags, and then a glibc setup hook to add idirafter. Sound good?

@vcunat
Copy link
Member

vcunat commented Oct 26, 2016

Or we could override libiconv.dev to only contain include/iconv.h and not other headers, or some similar hack.

@gridaphobe
Copy link
Contributor

Does that solve the original problem? If so, it sounds pretty clean!

@shlevy
Copy link
Member Author

shlevy commented Oct 27, 2016

It should, yeah.

@shlevy
Copy link
Member Author

shlevy commented Oct 27, 2016

See #19919. Will create a jobset.

@shlevy
Copy link
Member Author

shlevy commented Oct 27, 2016

#19919 merged into staging

@bzizou
Copy link
Contributor

bzizou commented Oct 28, 2016

cherry-picked the commit and tested against my branch -> ok for me, fixed the original problem!

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

No branches or pull requests

5 participants