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

libuv: 1.7.5 -> 1.9.0 #14973

Merged
merged 2 commits into from Apr 28, 2016
Merged

libuv: 1.7.5 -> 1.9.0 #14973

merged 2 commits into from Apr 28, 2016

Conversation

gilligan
Copy link
Contributor

Things done

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @Ralith, @cstrahan and @pikajude to be potential reviewers

@@ -81,5 +81,5 @@ in
}
//
mapAttrs (v: h: mkWithAutotools stable (toVersion v) h) {
v1_7_5 = "18x6cy2xn31am97vn6jli7kmb2fbp4c8kmv7jm97vggh0x55flsc";
v1_9_0 = "0sq8c8n7xixn2xxp35crprvh35ry18i5mcxgwh12lydwv9ks0d4k";
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to remove the older version? If we're just going to be supporting one version of libuv anyway, we might want to remove this machinery for providing multiple versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adnelson I guess I should have asked someone before I did the PR. I first exposed both versions and then changed it to only expose one default version. I basically was just unsure what the idiomatic/right nix way would be for this.

Thanks for your feedback. I will update the PR to expose both versions.

@gilligan
Copy link
Contributor Author

@adnelson @joachifm The ci check now failed twice with the same error:

curl: (22) The requested URL returned error: 500 Internal Server Error
/nix/store/hs8lf8v9qqfs9m3jqmm97nqg7wg9dqjw-xz-5.2.1/bin/xz: (stdin): File format not recognized
error: unexpected end-of-file
download of ‘https://cache.nixos.org/nar/1r11s9a025fbv7phbzihxvp5183piflw04hzdy4xwjza903ri7ki.nar.xz’ failed: No such file or directory
could not download ‘/nix/store/ivrsfvzcl2m5g1nxrgvfhc7d5ss01vmh-isnumber-1.0.0.tgz’ from any binary cache
fetching path ‘/nix/store/ivrsfvzcl2m5g1nxrgvfhc7d5ss01vmh-isnumber-1.0.0.tgz’ failed with exit code 1
fetching path ‘/nix/store/4cbax13dg2zmyg7xgjadvxsrbil0lf9q-jsonify-0.0.0.tgz’...
killing process 7446
killing process 7431
killing process 7432
killing process 7441
�[31;1merror:�[0m some substitutes for the outputs of derivation ‘/nix/store/h5gyc3f6icqp2jya956smlmrkb2yawrz-isnumber-1.0.0.tgz.drv’ failed (usually happens due to networking issues); try ‘--fallback’ to build derivation from source 

Is that indeed a networking problem? At least I don't have any other ideas about this. Just odd that it would happen twice in a row.

v1_7_5 = "18x6cy2xn31am97vn6jli7kmb2fbp4c8kmv7jm97vggh0x55flsc";
}
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just

mapAttrs (v: h: mkWithAutotools unstable (toVersion v) h) {
  v1_7_5 = "18x6cy2xn31am97vn6jli7kmb2fbp4c8kmv7jm97vggh0x55flsc";
  v1_9_0 = "0sq8c8n7xixn2xxp35crprvh35ry18i5mcxgwh12lydwv9ks0d4k";
}

@Ralith
Copy link
Contributor

Ralith commented Apr 26, 2016

Old versions should not be retained unless they are specifically required for other software to remain functional. Otherwise nixpkgs would fill up with unused cruft. We shouldn't retain 1.7.5 unless it's known for a fact that something breaks on 1.9, which is unlikely because libuv uses semantic versioning.

@adnelson
Copy link
Contributor

adnelson commented Apr 26, 2016

So if we don't want to retain multiple versions, let's not do the whole mapAttrs blah blah because it adds mental overhead to those reading the package definition.

Even better, we could remove the mkWithAutotools and mkWithoutAutotools and just use a simple streamlined call to mkDerivation.

@gilligan
Copy link
Contributor Author

@adnelson @Ralith alright so I will update the PR such that only 1.9.0 is exported

@gilligan
Copy link
Contributor Author

I updated the PR to only use 1.9.0 - I also removed the constrained of using an older version of automake since it built just fine with the most recent one.

@gilligan
Copy link
Contributor Author

I updated the rather ancient pyuv version and adjusted the patch.

@gilligan
Copy link
Contributor Author

Well sadly the job reached its time limit ;(

@gilligan
Copy link
Contributor Author

I tried testing this change with nox-review on my local nixos machine but it fails because of some node-gyp error building a nodejs library: https://gist.github.com/gilligan/b5f8a9d678580883e6930b31f3ca59ab

So I am stuck once again.

@FRidh
Copy link
Member

FRidh commented Apr 27, 2016

@gilligan try running nox-review with the -k flag and check what else is failing. If this is the only one, or one of the few, then likely this could be merged. Even so, cc the maintainer of node-gyp.
Maybe its @cillianderoiste or @Havvy ?

@gilligan
Copy link
Contributor Author

@FRidh here is the output of nox-review -k pr 14973 : https://gist.github.com/gilligan/53f84321d944c5c37ca970abe6aabf8c

@gilligan
Copy link
Contributor Author

@FRidh @adnelson are you OK with merging this ? I am pretty confident that whatever might fail in the CI run is either not related to the library update and/or is the result of the nodejs libraries in nix being all utterly outdated -- See also #14532

@adnelson
Copy link
Contributor

Excellent simplification. I'm fine merging it personally. The node stuff is pretty flaky in my experience...

@svanderburg svanderburg merged commit d76982e into NixOS:master Apr 28, 2016
@gilligan gilligan deleted the update-libuv branch April 28, 2016 10:48
@gilligan gilligan mentioned this pull request Apr 28, 2016
2 tasks
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

7 participants