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.

Sign up for GitHub

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

Jump to bottom

7903139: avoid jpackage in build.gradle #11

Closed
wants to merge 2 commits into from
Closed

7903139: avoid jpackage in build.gradle #11

wants to merge 2 commits into from
+34 −60

Conversation

sundararajana
Copy link
Member

@sundararajana sundararajana commented Mar 31, 2022 •
edited by openjdk bot
Loading

Using jlink with launcher option instead of jpackage. This ensures uniform layout of app dir ($app/bin/jextract script) on all platforms. Also this avoids the use of third-party jpackage grade plugin.

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jextract pull/11/head:pull/11
$ git checkout pull/11

Update a local copy of the PR:
$ git checkout pull/11
$ git pull https://git.openjdk.java.net/jextract pull/11/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11

View PR using the GUI difftool:
$ git pr show -t 11

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jextract/pull/11.diff

Sorry, something went wrong.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 31, 2022

👋 Welcome back sundar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Mar 31, 2022 •
edited
Loading

@sundararajana This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

7903139: avoid jpackage in build.gradle

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 31, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 31, 2022 •
edited
Loading

Webrevs

mcimadamore
mcimadamore reviewed Mar 31, 2022
View reviewed changes
Copy link
Contributor

@mcimadamore mcimadamore left a comment

Choose a reason for hiding this comment

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

Looks good, the only nit is that I can see few JDK-specific launchers in the bin folder which can be confusing - e.g. javac, java, serialver etc. Would it be possible to drop them?

@mcimadamore
Copy link
Contributor

Looks good, the only nit is that I can see few JDK-specific launchers in the bin folder which can be confusing - e.g. javac, java, serialver etc. Would it be possible to drop them?

Uhmmm - seems like jlink doesn't allow for that, and if we clean up all executables, then the jextract script fails because it can't find "java".

@sundararajana
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 31, 2022
@openjdk
Copy link

openjdk bot commented Mar 31, 2022

@sundararajana
Your change (at version 373aaf4) is now ready to be sponsored by a Committer.

@mcimadamore
Copy link
Contributor

I believe README needs changes.
@overheadhunter, does this work with your plugin?

@overheadhunter
Copy link

@overheadhunter, does this work with your plugin?

So far one needs to manually specify the exact path of the jextract "binary" (launcher script should behave the same) anyway. I don't expect any problems. If there are, I'll adjust the plugin, so please go ahead! 🙂

@mcimadamore
Copy link
Contributor

@overheadhunter, does this work with your plugin?

So far one needs to manually specify the exact path of the jextract "binary" (launcher script should behave the same) anyway. I don't expect any problems. If there are, I'll adjust the plugin, so please go ahead! slightly_smiling_face

Ok, thanks. What this change does, is that it makes the build output more regular, so your plugin can just take a variable that points to "jextract home".

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Mar 31, 2022
mcimadamore
mcimadamore reviewed Mar 31, 2022
View reviewed changes
README.md

To run the `jextract` tool, simply run the `jextract` command in the `bin` folder (again, the exact location of the binary might vary slightly depending on the platform, e.g. on macOs `build/jextract.app/Contents/MacOS/jextract` ):
After building, there should be a new `jextract` folder under `build`.
To run the `jextract` tool, simply run the `jextract` command in the `bin` folder:
Copy link
Contributor

Choose a reason for hiding this comment

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

should say jextract.bat for windows (e.g. in parens) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... "jextract' command is what we say. On Windows jextract.exe, jextract.bat both will be called "jextract" command (because user doesn't run with the extension), right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, .bat can be dropped

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@sundararajana
Copy link
Member Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 31, 2022
@openjdk
Copy link

openjdk bot commented Mar 31, 2022

@sundararajana
Your change (at version 92dec34) is now ready to be sponsored by a Committer.

@mcimadamore
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Mar 31, 2022

Going to push as commit fd03cfe.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 31, 2022
@openjdk openjdk bot closed this Mar 31, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 31, 2022
@openjdk
Copy link

openjdk bot commented Mar 31, 2022

@mcimadamore @sundararajana Pushed as commit fd03cfe.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@sundararajana sundararajana deleted the CODETOOLS-7903139 branch March 31, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JornVernee JornVernee

@mcimadamore mcimadamore

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sundararajana @mcimadamore @overheadhunter @JornVernee