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

kill libiconvOr* #6166

Merged
merged 2 commits into from
Feb 17, 2015
Merged

kill libiconvOr* #6166

merged 2 commits into from
Feb 17, 2015

Conversation

gridaphobe
Copy link
Contributor

The pure-darwin branch doesn't include libiconv in its libc the way glibc does, nor does it let nixpkgs link against apple's libiconv (for obvious reasons). So we've had to make a lot of packages more explicit about their dependence on libiconv, and have merged all the libiconvOr variants into a single libiconv that points to glibc on linux and libiconv on darwin.

An alternative approach would be to just bundle libiconv with the darwin stdenv, but @copumpkin has strong feelings against this 😄.

The downside to this PR is that it will cause major rebuilding on linux.

/cc @shlevy @edolstra

@gridaphobe
Copy link
Contributor Author

Oh also, if we decide to merge this, I'd also like to remove the libintlOrEmpty similarly.

@copumpkin
Copy link
Member

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.

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.

@vcunat
Copy link
Member

vcunat commented Feb 5, 2015


NIX_LDFLAGS = stdenv.lib.optionalString (libiconvOrNull != null) "-L${libiconvOrNull}/lib -liconv";
NIX_LDFLAGS = "-L${libiconv}/lib -liconv";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -L is unnecessary here, given the buildInputs

@copumpkin
Copy link
Member

@vcunat damn, that's annoying. We could add a dummy linker script (posing as libiconv.so) to glibc, or just bite the bullet and grow the footprint of our stdenv/libc.

@gridaphobe
Copy link
Contributor Author

Let me also check whether the -liconv is actually necessary. It seems a bit odd that autoconf wouldn't find it for us as long as libiconv is in the buildInputs.

@vcunat
Copy link
Member

vcunat commented Feb 5, 2015

I would expect that in most cases the needed flags are detected and used automatically, but I know little about darwin stuff (never had one). In other cases, it can be e.g. lib.optionalString stdenv.isDarwin "-liconv". I doubt anyone tested it elsewhere anyway...

@gridaphobe
Copy link
Contributor Author

@vcunat right, and at least in the case of gnugrep it looks like autoconf is smart enough to find the nix libiconv.

@gridaphobe
Copy link
Contributor Author

@vcunat i think the main reason for making libiconv point to glibc on linux is so people can write things like ${libiconv}/lib and get a sensible result.

@shlevy
Copy link
Member

shlevy commented Feb 5, 2015

👍 on this, and generally agree with @copumpkin on explicitness being desirable here.

@shlevy
Copy link
Member

shlevy commented Feb 5, 2015

(Haven't looked through the details, just 👍 for the overall)

@gridaphobe
Copy link
Contributor Author

@shlevy shall we fire up a hydra job for it? (I'm assuming the additional buildInputs on linux will force a number of rebuilds :-/)

@shlevy
Copy link
Member

shlevy commented Feb 5, 2015

Conflicts:
	pkgs/applications/networking/mailreaders/sup/default.nix
	pkgs/development/compilers/ghc/7.8.3-binary.nix
	pkgs/development/interpreters/php/5.3.nix
	pkgs/development/interpreters/ruby/patches.nix
	pkgs/development/libraries/cairo/default.nix
	pkgs/development/libraries/poppler/default.nix
	pkgs/top-level/all-packages.nix
@copumpkin
Copy link
Member

This doesn't actually seem that risky. What are the major downsides from merging it now? Mostly just in a rush because it's making other merges from pure-darwin more painful.

@gridaphobe
Copy link
Contributor Author

The downside is that it triggers a mass-rebuild, and it would be nice to let hydra catch up.

@copumpkin
Copy link
Member

Oh, I just meant that we could merge it into staging and be done with it.

@gridaphobe
Copy link
Contributor Author

Fine by me, I don't have much of an opinion on the staging vs individual jobsets debate.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

I'm not a big fan of staging really, it takes longer for things to get into master than otherwise and splits development focus (should I build my work on top of staging or master?). What does merging into staging gain us here?

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, the advantage of staging is that binaries are already available when that branch gets merged into master, because Hydra has already built them. If you go to master directly, then everyone who doesn't have the ability to re-compile the whole world is essentially blocked from updating their system (or even individual packages) for the next few days.

Another important reason for using staging is that we, as a community, set that branch up to be used for the collecting mass-rebuilding changes, and there is value in adhering to the rules that a community gives itself, even if those rules might be inconvenient for an individual at a given time, because if individuals frequently don't play by those rules, then they set an example for others to do the same thing, and eventually the community might fall apart in the sense that it no longer even tries to act as a collective.

In other words, staging is useful from both a technical and a social point of view.

@bluescreen303
Copy link
Contributor

while I agree with @peti , I do think that it often takes a bit long before staging is merged.The original meaning was indeed to give hydra some headstart to build binaries and keep experimental upgrades on separate branches. Perhaps this distinction blurs a bit from time to time.

@vcunat
Copy link
Member

vcunat commented Feb 9, 2015

@bluescreen303: noone prevents interested people from merging staging (or a slightly older version of it) as soon as most of it is rebuilt. Typically there aren't any big regressions in that branch.

IMHO most of the delays are just because Hydra needs lots of time to do full rebuilds, which is also helped by staging, because it bulks several mass-rebuild changes in one rebuild.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

@peti I'm not suggesting to merge this before hydra has had a chance to build it. The idea is to make a jobset for the PR and then merge when it's done.

If the binaries are available, how exactly is the community harmed here?

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, the consensus -- as I remember it -- is that patches should be tested (i.e. in their own build jobs), and only after those tests have succeeded, they should go to staging. The purpose of stagingis not to test the patches, it's only purpose is to batch multiple mass-rebuilding changes, i.e. staging should always be in a state where it can be merged immediately, if we want to. This is a very reasonable policy, IMHO, and I don't understand why seem to be opposed to it.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

I don't understand why we need to wait. In principle, combining the changes obfuscates where breakages come from and adds an extra level of indirection. In practice, staging (and stdenv-updates, which we got rid of for a reason and then revived it not a few months later) actually delays changes getting in, both because there is no clear point at which we can say "yes, we're done, let's merge" and because things break on top of each other and no one person is responsible. Unless you're proposing something that changes the situation as it's been for years, why should I think things will suddenly be different now?

And what consensus?

And, again, what is hurt by going this route? The binaries will be available before any merge to master, the jobs will be tested before any merge to master.

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, personally, I'd rather download my whole system only once, if possible, instead of re-downloading everything from scratch with every low-level change that's being made. I don't share your perception of urgency.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

These changes have been brewing for months and the longer they stay off master, the longer we don't have working darwin on nixpkgs and the further divergence there is, creating more work to merge it in. Nothing requires you to use the latest stdenv when testing new non-stdenv changes, and the whole reason we have a stable branch is so that we can move quickly on master. Why make everyone batch it up, you can batch it up at your own convenience by simply pinning stdenv where you like it until you're ready for a change.

Changes have sat in staging/stdenv-updates for months to years. As long as these changes are not in master, we can't reasonably move forward with the work of upstreaming pure-darwin.

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, Darwin support is not one of my core interests because, frankly, I don't own a Mac. I am totally in favor of supporting the platform, but whether the new Darwin stdenv arrives in master today, tomorrow, or in a month from now makes no difference to me. What does make a difference to me, however, is if I have to re-download my entire system every few couple of days, and I really don't want that. So please do me the favor and try to batch stdenv changes.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

Why can't you batch them on your end?

Frankly, for a discussion that started off with you accusing me of "not playing by the community rules", it's interesting that you've ended up by explicitly placing your personal needs against a significant and growing subset of the community because you don't care about it.

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, I don't want to create my own private master branch. I realize it's possible, but I just don't want to. I want to follow the master branch we already have.

Also, please note that it's not true to say that I don't care about the efforts to bring a pure Darwin stdenv into Nixpkgs. I do care! I am completely in favor of the effort and I'll gladly contribute whatever I can to get it done. I don't agree, however, that merging stdenv changes directly to master -- without going through staging -- is in any way important for the Darwin effort. That is your interpretation, but I don't share it.

@copumpkin
Copy link
Member

The issue is that keeping a long-lived fork working against a master that
breaks it all the time is a pretty painful job, and I'm getting sick of it
:) a few extra days isn't the end of the world, but it's frustrating to
have to wait when there are dependencies between the PRs.

On Monday, February 9, 2015, Peter Simons notifications@github.com wrote:

@shlevy https://github.com/shlevy, I don't want to create my own
private master branch. I realize it's possible, but I just don't want to. I
want to follow the master branch we already have.

Also, please note that it's not true to say that I don't care about the
efforts to bring a pure Darwin stdenv into Nixpkgs. I do care! I am
completely in favor of the effort and I'll gladly contribute whatever I can
to get it done. I don't agree, however, that merging stdenv changes
directly to master -- without going through staging -- is in any way
important for the Darwin effort. That is your interpretation, but I don't
share it.


Reply to this email directly or view it on GitHub
#6166 (comment).

@peti
Copy link
Member

peti commented Feb 9, 2015

@copumpkin, I believe everyone agrees that staging isn't merged frequently enough. My suggestion is that we make an effort to change that, i.e. we make sure it gets merged at least once or twice a month. I'll begin by taking the responsibility for merging the branch we have right now (just posted to nix-dev about it).

This should allow us a work flow where individual patches are tested (possibly in separate Hydra jobs) and moved to staging once we're confident they work. And then, it shouldn't take more than 5-7 days to have them in master. If it turns out that this precedure doesn't work for some reason, then we'll figure out something else. But I would like to give a shot. We, as a community, really need a working scheme for updating stdenv.

@gridaphobe
Copy link
Contributor Author

Why not have hydra automatically merge staging into master once it finishes building a jobset? (with some logic to prevent merging commits that causes massive breakage of course..)

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

You don't have to keep a separate branch. Just override stdenv to an older checkout.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

We have this ability to do really flexible things with nix, and instead we keep to a model that has proven to slow things down considerably. Show me that the staging process has actually been streamlined, or that it provides a benefit that can't be achieved by just setting a packageOverride, or that I'm really going agains the entire will of the community here, and I'll back off. But frankly this seems like a case where we're slowing down improvements so that people can stick to outdated workflows.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

Also, as for "we make an effort to change that", in my years in the community I've seen many efforts to change our workflows as a group, many of which have been initiated by me, with pretty much no success. At this point, I don't believe anything that is not sustainable by one person doing something themselves is viable in the nix community. I'm willing to create these jobsets, verify they're working, and merge them in when they're ready for issues I'm directly invested in (though I'd prefer it if @rbvermaa or @edolstra would grant more people hydra perms). I'm not willing to wrangle staging, given that I think the model is completely broken and makes things harder than they should be for no compelling reason. So if the end result of this is that all of these changes have to go through staging, I have basically no confidence that it will get done in any kind of timely manner.

@copumpkin
Copy link
Member

Perhaps this discussion should move to the mailing where it'll get more
visibility?

On Monday, February 9, 2015, Shea Levy notifications@github.com wrote:

Also, as for "we make an effort to change that", in my years in the
community I've seen many efforts to change our workflows as a group, many
of which have been initiated by me, with pretty much no success. At this
point, I don't believe anything that is not sustainable by one person doing
something themselves is viable in the nix community. I'm willing to create
these jobsets, verify they're working, and merge them in when they're ready
for issues I'm directly invested in (though I'd prefer it if @rbvermaa
https://github.com/rbvermaa or @edolstra https://github.com/edolstra
would grant more people hydra perms). I'm not willing to wrangle staging,
given that I think the model is completely broken and makes things harder
than they should be for no compelling reason. So if the end result of this
is that all of these changes have to go through staging, I have basically
no confidence that it will get done in any kin d of timely manner.


Reply to this email directly or view it on GitHub
#6166 (comment).

@peti
Copy link
Member

peti commented Feb 9, 2015

@shlevy, I feel like I've made my case, and I'd rather not discuss this subject any longer. Let us agree to disagree.

@shlevy
Copy link
Member

shlevy commented Feb 9, 2015

@shlevy
Copy link
Member

shlevy commented Feb 12, 2015

Going to merge this into staging, hydra can't handle multiple stdenv rebuilds in parallel.

@copumpkin
Copy link
Member

I certainly won't complain :)

On Wednesday, February 11, 2015, Shea Levy notifications@github.com wrote:

Going to merge this into staging, hydra can't handle multiple stdenv
rebuilds in parallel.


Reply to this email directly or view it on GitHub
#6166 (comment).

@shlevy
Copy link
Member

shlevy commented Feb 12, 2015

127efcd

@copumpkin
Copy link
Member

Is this not in master yet?

@copumpkin
Copy link
Member

I guess not; I think staging got merged to master right before our two changes went in 😦

@peti
Copy link
Member

peti commented Feb 17, 2015

@copumpkin, I'll merge staging again quickly. I'm just waiting for http://hydra.nixos.org/jobset/nixpkgs/staging to complete building the most recent version.

@copumpkin
Copy link
Member

@peti great, thanks! Somehow I'd gotten my hopes up and thought that our changes had already been built 😄 looks like we're almost there. If you're asleep later, shall I just merge it? assuming nothing bad happens between now and later

@shlevy shlevy merged commit 472feaf into NixOS:master Feb 17, 2015
@peti
Copy link
Member

peti commented Feb 18, 2015

@copumpkin, I decide to merge staging whenever (a) the branch hasn't gotten any new commits for a couple of days and (b) the current HEAD has been build successfully by Hydra. Feel free to use the same heuristic. 😄

@vcunat
Copy link
Member

vcunat commented Feb 18, 2015

In case (a) doesn't apply for a long time, one can merge some older successful version of staging.

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

Successfully merging this pull request may close these issues.

None yet

6 participants