-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Misc static improvements (mainly ocaml and a regression linked to b0b5ef7286dca098f40f5075175105c3c0dfbe05) #133008
Conversation
e77f227
to
fcf470a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't lnow much about the stdenv stuff, but other than 1 comment LGTM. Have not tried building.
@@ -7,7 +7,7 @@ | |||
, gnutlsSupport ? false, gnutls ? null | |||
, wolfsslSupport ? false, wolfssl ? null | |||
, scpSupport ? zlibSupport && !stdenv.isSunOS && !stdenv.isCygwin, libssh2 ? null | |||
, gssSupport ? with stdenv.hostPlatform; !( | |||
, gssSupport ? with stdenv.hostPlatform; ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this change. Why invert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the change:
gss support is enabled when: on windows, or building a static library or cross compiling to darwin. Given the issues linked in comment, this looks backward to me.
Can we set some strictDeps to prevent regressions in the future? |
done |
I like where are you are going with this :). Two things, and then it's good
|
I agree with 1. but I have no idea how to implement that... would you have a hint?
12 août 2021 04:10:13 John Ericson ***@***.***>:
I like where are you are going with this :). Two things, and then it's good
1. > You should fix *dontAddStaticConfigureFlags* by instead making the stdenv adapter also override the definition *overrideAttrs*. The problem is *overrideAttrs* closes over the old *mkDerivation*. I don't like seeing new stuff in *setup.sh* because that file has a terrible history of only growing more complex, never shrinking.
2. > *ocamlFixPackage* *ocamlStaticAdapter* barely do anything now, can you just kill them off entirely? I want to get rid of the stdenv adapter completely and you've already done the hard underlying work of getting us closer :).
…
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub[#133008 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ADADGA2SUHLOQKJF3VFI4P3T4MUQDANCNFSM5BXKRS5A].
Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAD8AAAA/CAYAAABXXxDfAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAmSURBVGiB7cEBDQAAAMKg909tDwcUAAAAAAAAAAAAAAAAAAAAJwY+QwABivJx1AAAAABJRU5ErkJggg==###24x24:true###][Image de pistage][https://github.com/notifications/beacon/ADADGAYIOZGK2RRTSBQEF7TT4MUQDA5CNFSM5BXKRS5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGV5ZMJQ.gif]
|
f59c05b
to
652d287
Compare
I attempted something. I suspect that topkg-based packages require more work for real cross to work. Compile tested by
|
@GrahamcOfBorg eval the ci failure is a flaky test in aws-c-common: #133857 (comment) |
@symphorien Nice attempt. That does work, but I think it will be even easier if we make
and it will "just work", no Mind giving that a shot? |
oh it might have to be
The override would be
and default arg
|
I ended up just doing the adapter fixing part of this. I'll put up a PR tomorrow which can just go to master. Then this can just worry about the individual packages on top. |
fe73691
to
a17fc03
Compare
Looks great! |
Is this tested enough to merge? |
I added a commit getting rid of more things in the overlay. I also built that stuff. |
397d2c9
to
0ebaceb
Compare
Sorry I made a typo, should be fixed now. |
findlib's setup hook is now a preConfigure setup hook, so make sure to run it
0ebaceb
to
9046258
Compare
I checked that ocamlbuild and pkgsStatic.ocamlbuild build, execute and are from the expected platform. i won't have time to check much more. |
I built those two in the end after the stuff I changed. Will merge this once ofborg finally signs off! |
Thanks again! |
FYI, I'm not sure where in |
Builds again after commit 4215591. |
This commit: b1b2305#diff-db67b092b9f267a4586cc73d414de28f03cc5a5d774d23284c4bc745420ab8a7 breaks libbfd on x86 darwin on master from what I can tell. Without the added 'libbfd: add strictDeps = true' line, it compiles fine.
|
Did this break OCaml 4.09 on darwin? https://hydra.nixos.org/build/152737288/nixlog/3/tail Did this break OCaml on darwin aarch64? https://hydra.nixos.org/build/157047473/nixlog/2/tail |
This kind of error is caused by a missing
(see https://nixos.org/manual/nixpkgs/stable/#strictoverflow ) |
Motivation for this change
Bulding static ocaml packages
Most of the PR should be uncontroversial, but there is a change to stdenv linked to a regression in b0b5ef7
The gist is
.overrideAttrs (o: { dontAddStaticConfigureFlags = true })
does not work becauseo.configureFlags
already contains the faulty flags and they will not be remove any more. You can see this on current nixpkgs with:which has both
dontAddStaticConfigureFlags
set to 1 and--disable-shared
inconfigureFlags
.(this affects all of ocaml packages)
Proposed solution is to move the logic from nix (which is already complex with all the overrideScope' at play) to bash in stdenv builder.
cc @alyssais for dontAddStaticConfigureFlags and @veprbl for ocaml part.
Tested with
This is a mass rebuild, so please refrain from bikeshedding on spacing as it takes half a day to rebuild.