-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
ant: use upstream launcher script #359451
ant: use upstream launcher script #359451
Conversation
postPatch = '' | ||
# Hard-code immutable paths | ||
substituteInPlace bin/ant \ | ||
--replace-fail '$JAVA_HOME' '${jre.home}' \ |
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.
Do we actually need jre in buildInputs, if we just substitute it like this?
(haven't checked)
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 don't think so. I did it because that's what the convention is, as far as I understand it; happy to remove it if that isn't the case.
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.
Also, will this change make it so that we don't actually need to add both jdk and ant into nativeBuildInputs when using ant?
Also also, jdk
vs jre
? (I'd say jdk
)
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.
From my testing, if we use jdk
, that is the case, but setting it to jdk
makes it difficult to use a different version of Java without overriding ant
.
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.
Is there a reason that we shouldn't have consumers explicitly specify a JDK?
I'm not sure if using I would probably either use Here's the unsubstituted launcher script for easier viewing: https://gist.github.com/TomaSajt/485fc88337e768e2d065cc61eb0c4560 |
5547f68
to
8c6c862
Compare
8c6c862
to
bf30e44
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bf30e44
to
194a89e
Compare
194a89e
to
a6d3631
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a6d3631
to
0272a6f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
20c091f
to
a270f7a
Compare
|
||
chmod +x $out/bin/ant | ||
# Wrap the wrappers. | ||
for wrapper in ant runant.py runant.pl; do |
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 unsure about whether we should consider including perl
and python
in nativeBuildInputs
so that these scripts have their shebangs patched; it potentially bloats the closure, but users might expect to be able to execute these out-of-the-box.
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.
They would go in buildInputs
. But at least perl ends up in the default system PATH anyhow.
This comment was marked as outdated.
This comment was marked as outdated.
0ac4c03
to
ba3ddc6
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.
LGTM.
I assume the Java version used for the wrappers is not of significance for ant users, right?
No, it shouldn't be. |
The rebuild amount doesn't justify staging. |
OfBorg's rebuild count isn't accurate. It's approximately 800 cumulative rebuilds (a few weeks ago; not much has changed since then, just a few leaf packages being dropped, unless I missed a larger drop of a non-leaf package): #357162 (comment) |
ba3ddc6
to
f203e8b
Compare
CI failures look unrelated. |
f203e8b
to
95ff196
Compare
Rebased onto common ancestor of |
95ff196
to
ccd553d
Compare
Warning
Superseded by #364094.
Let's use Ant's upstream launcher script. It handles far more edge-cases than ours.
Instead of writing our own, let's simply hard-code values into the launcher that are known in advance.
One small step towards #353512. Closes #4105, because a path to the JRE is one of the things we now explicitly set. Something similar first attempted by @linsui in #266313.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.