-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
ldc: 1.17.0 -> 1.20.1 #85488
ldc: 1.17.0 -> 1.20.1 #85488
Conversation
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.
Thanks a lot for this!
@@ -0,0 +1,10 @@ | |||
{ callPackage }: | |||
callPackage ./binary.nix { | |||
version = "1.19.0"; |
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.
Was there a specific reason why the previous version is used for bootstrapping?
I can think of no technical reason but it would make it even easier if the same version is used for bootstrapping.
This way only one version specifier needs to exist.
pkgs/top-level/all-packages.nix
Outdated
@@ -4554,6 +4554,7 @@ in | |||
|
|||
lalezar-fonts = callPackage ../data/fonts/lalezar-fonts { }; | |||
|
|||
ldcBootstrap = callPackage ../development/compilers/ldc/bootstrap.nix { }; |
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 am not sure if it is good to provide the bootstrap package this way.
It should not be available to the outside because it is not a "nice" package but a hackish one and is only needed for building the real package which provides the same functionality.
I can only imagine a testing scenario where having the bootstrap package available that way but this is highly speculative as I don't have a concrete example.
Even if such an example could be found it's for sure quite rare and can be added by a developer temporarily without polluting the namespace.
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 agree, ldcBootstrap
is an implementation detail that shouldn't be exposed.
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.
Could advertise it as ldc_bin
or ldc.bootstrap
, probably.
In any case, it should be exposed in some way, otherwise how one can override 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.
@veprbl What do you mean?
It could simply be done by calling binary.nix directly from generic.nix like it was done in the previous solution [1].
This way bootstrap.nix and ldcBootstrap in all-packages.nix would not be necessary any more.
[1] https://github.com/NixOS/nixpkgs/blob/19.09/pkgs/development/compilers/ldc/default.nix#L13
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.
removed.
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.
@ThomasMader If I need to override bootstrap ldc in an overlay, how do I do that?
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.
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.
Yes, this looks better. It would also help if the original bootstrap value was exposed. e.g. add passthru = { inherit ldcBootstrap; }
.
@ThomasMader Feel free to ping me for merge on this PR and the dmd one. |
@worldofpeace It's WIP because I'm getting a failure during check phase on my local MacBook. Need to dig in. |
@lionello What failure? Can you elaborate? Mostly I can't help without digging in too but maybe we are lucky. |
@ThomasMader Actually, it's |
@worldofpeace @ThomasMader Locally, this now builds successfully on macOS and NixOS with tests passing. |
@GrahamcOfBorg build ldc |
@worldofpeace would it be possible to get this into 20.03 or is the release already frozen? |
We've been go for the final release for a week https://discourse.nixos.org/t/go-no-go-meeting-nixos-20-03-markhor/, so we're very past the freeze. |
@GrahamcOfBorg build ldc |
I issued the build command @ThomasMader because I didn't see it build on macos for you. So I guess that means you aren't in "trusted users" in ofborg, which basically means you can build on darwin plus other minor stuff. |
I used to be able to build on Darwin too. Never mind. |
Thanks! Please check the dmd one as well: #82694 |
This broke
|
Oh, did we need to merge dmd together? |
Yes merging #82694 should fix those problems. |
Motivation for this change
Current
ldc
uses an extremely old version ofldc
0.x to bootstrap the latestldc
1.x, causing more and more issues as the gap between the two versions widens.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)