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

ant: use upstream launcher script #359451

Conversation

tomodachi94
Copy link
Member

@tomodachi94 tomodachi94 commented Nov 26, 2024

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

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

postPatch = ''
# Hard-code immutable paths
substituteInPlace bin/ant \
--replace-fail '$JAVA_HOME' '${jre.home}' \
Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Member Author

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?

@TomaSajt
Copy link
Contributor

TomaSajt commented Nov 26, 2024

I'm not sure if using substituteInPlace is a good idea.
(${VAR} and $VAR mean the same thing, but only one of them is replaced).

I would probably either use wrapProgram $out/lib/ant/bin --set ANT_HOME "..." or just put export ANT_HOME="..." after the shebang in the script.

Here's the unsubstituted launcher script for easier viewing: https://gist.github.com/TomaSajt/485fc88337e768e2d065cc61eb0c4560

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from 5547f68 to 8c6c862 Compare November 27, 2024 00:48
@github-actions github-actions bot added the 6.topic: java Including JDK, tooling, other languages, other VMs label Nov 27, 2024
@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from 8c6c862 to bf30e44 Compare November 27, 2024 00:50
@tomodachi94

This comment was marked as outdated.

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from bf30e44 to 194a89e Compare November 27, 2024 04:13
@tomodachi94 tomodachi94 linked an issue Nov 27, 2024 that may be closed by this pull request
@tomodachi94 tomodachi94 requested a review from TomaSajt November 27, 2024 21:44
@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from 194a89e to a6d3631 Compare November 27, 2024 22:11
@tomodachi94

This comment was marked as outdated.

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from a6d3631 to 0272a6f Compare November 28, 2024 03:10
@tomodachi94 tomodachi94 marked this pull request as draft November 28, 2024 06:04
@tomodachi94

This comment was marked as outdated.

@tomodachi94 tomodachi94 added the 2.status: work-in-progress This PR isn't done label Nov 28, 2024
@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch 2 times, most recently from 20c091f to a270f7a Compare November 28, 2024 09:54

chmod +x $out/bin/ant
# Wrap the wrappers.
for wrapper in ant runant.py runant.pl; do
Copy link
Member Author

@tomodachi94 tomodachi94 Nov 28, 2024

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.

Copy link
Member

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.

@tomodachi94 tomodachi94 removed the 2.status: work-in-progress This PR isn't done label Nov 28, 2024
@tomodachi94 tomodachi94 marked this pull request as ready for review November 28, 2024 09:57
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 labels Nov 29, 2024
@tomodachi94 tomodachi94 added the 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed label Nov 29, 2024
@tomodachi94

This comment was marked as outdated.

@tomodachi94 tomodachi94 added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed labels Nov 30, 2024
@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch 3 times, most recently from 0ac4c03 to ba3ddc6 Compare November 30, 2024 00:24
@tomodachi94 tomodachi94 removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 30, 2024
Copy link
Member

@FliegendeWurst FliegendeWurst left a 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?

@ofborg ofborg bot requested a review from FliegendeWurst November 30, 2024 15:09
@tomodachi94
Copy link
Member Author

No, it shouldn't be.

@SuperSandro2000
Copy link
Member

The rebuild amount doesn't justify staging.

@tomodachi94
Copy link
Member Author

tomodachi94 commented Nov 30, 2024

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)

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from ba3ddc6 to f203e8b Compare December 2, 2024 19:17
@tomodachi94
Copy link
Member Author

CI failures look unrelated.

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from f203e8b to 95ff196 Compare December 7, 2024 23:37
@tomodachi94
Copy link
Member Author

Rebased onto common ancestor of master and staging, with the hope that CI will succeed.

@tomodachi94 tomodachi94 force-pushed the enhance/ant/use-upstream-wrapper branch from 95ff196 to ccd553d Compare December 11, 2024 01:59
@github-actions github-actions bot removed the 6.topic: java Including JDK, tooling, other languages, other VMs label Dec 11, 2024
@tomodachi94 tomodachi94 deleted the enhance/ant/use-upstream-wrapper branch December 11, 2024 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ant: missing dependency on JRE, needed at runtime
4 participants