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

8239708: Split basics.m4 into basic.m4 and util.m4 #1277

Closed
wants to merge 4 commits into from

Conversation

gdams
Copy link
Member

@gdams gdams commented Jul 28, 2022

The main conflict is the lack of the WSL backport (8215445: Enable building for Windows in WSL) Something which doesn't apply particularly cleanly and isn't required to achieve the MSYS2 backport that I'm trying to reach.

This backport will also considerably help many build/toolchain backports going forwards as this one nearly always trips me up when I backport patches.

This backport will allow me to more easily backport 8257679: Improved unix compatibility layer in Windows build (winenv).

@RealCLanger / @GoeLin I suggest that this is run through the full SAP nightlies before this is merged.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issues

  • JDK-8239708: Split basics.m4 into basic.m4 and util.m4
  • JDK-8213239: Configure cannot handle command overrides with arguments
  • JDK-8206125: [windows] cannot pass relative path to --with-boot-jdk

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev pull/1277/head:pull/1277
$ git checkout pull/1277

Update a local copy of the PR:
$ git checkout pull/1277
$ git pull https://git.openjdk.org/jdk11u-dev pull/1277/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1277

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/1277.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 28, 2022

👋 Welcome back gdams! 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 openjdk bot changed the title Backport 63f939636a5fe9871f5f9057c93ed5b1a9924265 8239708: Split basics.m4 into basic.m4 and util.m4 Jul 28, 2022
@openjdk
Copy link

openjdk bot commented Jul 28, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Jul 28, 2022
@mlbridge
Copy link

mlbridge bot commented Jul 28, 2022

Webrevs

@RealCLanger
Copy link
Contributor

I've added it to our test queue, let's see what happens 😄

@RealCLanger
Copy link
Contributor

RealCLanger commented Aug 1, 2022

I've finally managed to review this backport a bit more in depth.

First of all, there are some other changes of which you bring in some parts but not everything. There are a few which are good candidates for backports as well, so I would recommend to do so before merging this patch. Namely:

https://bugs.openjdk.org/browse/JDK-8218413 (openjdk/jdk@9efdb33)

https://bugs.openjdk.org/browse/JDK-8217032 (openjdk/jdk@4f45b5f)

https://bugs.openjdk.org/browse/JDK-8221907 (openjdk/jdk@0974861)

https://bugs.openjdk.org/browse/JDK-8233712 (openjdk/jdk@753c58b)

Then, from https://bugs.openjdk.org/browse/JDK-8211724 (openjdk/jdk@d345832) you bring a little hunk: BASIC_REQUIRE_PROGS(MKDIR, [gmkdir mkdir]). That's ok and the rest of the change is too much, so that's alright.

https://bugs.openjdk.org/browse/JDK-8206125 (openjdk/jdk@7ae384b) is partly implemented in your change. Some hunks in new util_windows.m4 are missing, however. Please make sure it's completely contained in your backport and add the issue to this PR then.

https://bugs.openjdk.org/browse/JDK-8213239 (openjdk/jdk@68dbbf5) is contained in your PR. Please mark as backported by adding it to this PR.

https://bugs.openjdk.org/browse/JDK-8240972: Make sure that the changes don't get lost. The implementation file is a different before and after this change. See: f771367 vs. openjdk/jdk@e30b89e

Then, the backport of https://bugs.openjdk.org/browse/JDK-8285728 (f00776b) must not be lost which it currently would.

And finally, we should thoroughly check whether the fix for 8226346: Build better binary builders (non-public bug, contained in a CPU release) is still intact after this PR. The implementation file is also different, see 3e9a9c9 vs. openjdk/jdk@4df99aa

@gdams
Copy link
Member Author

gdams commented Aug 4, 2022

https://bugs.openjdk.org/browse/JDK-8218413 (openjdk/jdk@9efdb33)

Done via #1300

https://bugs.openjdk.org/browse/JDK-8217032 (openjdk/jdk@4f45b5f)

This backport has too many dependencies and would require a much larger refactor so I've stripped the pandoc stuff out.

https://bugs.openjdk.org/browse/JDK-8221907 (openjdk/jdk@0974861)

Done via #1301

https://bugs.openjdk.org/browse/JDK-8233712 (openjdk/jdk@753c58b)

Done via #1308

https://bugs.openjdk.org/browse/JDK-8206125 (openjdk/jdk@7ae384b) is partly implemented in your change. Some hunks in new util_windows.m4 are missing, however. Please make sure it's completely contained in your backport and add the issue to this PR then.

I've added the remaining chunk, issue added to backport

https://bugs.openjdk.org/browse/JDK-8213239 (openjdk/jdk@68dbbf5) is contained in your PR. Please mark as backported by adding it to this PR.

Issue added to backport

https://bugs.openjdk.org/browse/JDK-8240972: Make sure that the changes don't get lost. The implementation file is a different before and after this change. See: f771367 vs. openjdk/jdk@e30b89e

I've reimplemented this

Then, the backport of https://bugs.openjdk.org/browse/JDK-8285728 (f00776b) must not be lost which it currently would.

I've reimplemented this

And finally, we should thoroughly check whether the fix for 8226346: Build better binary builders (non-public bug, contained in a CPU release) is still intact after this PR. The implementation file is also different, see 3e9a9c9 vs. openjdk/jdk@4df99aa

I've carried over the change to basics.m4. This should be okay now

@openjdk-notifier
Copy link

@gdams Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@gdams
Copy link
Member Author

gdams commented Aug 4, 2022

/issue add JDK-8213239

@gdams
Copy link
Member Author

gdams commented Aug 4, 2022

/issue add JDK-8206125

@openjdk
Copy link

openjdk bot commented Aug 4, 2022

@gdams
Adding additional issue to issue list: 8213239: Configure cannot handle command overrides with arguments.

@openjdk
Copy link

openjdk bot commented Aug 4, 2022

@gdams
Adding additional issue to issue list: 8206125: [windows] cannot pass relative path to --with-boot-jdk.

@openjdk-notifier
Copy link

@gdams Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

Copy link
Contributor

@RealCLanger RealCLanger left a comment

Choose a reason for hiding this comment

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

Looks merely good now. I have two minor nits and I was wondering why you did not complete the backport of the pandoc change 8217032 (#1315). I think we figured out that we should rather not do the larger pandoc changes but this one could still be nice to take over.
In autoconf/basic_tools.m4, the AC_DEFUN_ONCE([BASIC_SETUP_PANDOC] stuff is missing currently which is contained in the upstream change for 8239708.

make/autoconf/basic_tools.m4 Outdated Show resolved Hide resolved
make/autoconf/basic.m4 Show resolved Hide resolved
@gdams gdams changed the base branch from master to pr/1352 September 1, 2022 08:41
@openjdk
Copy link

openjdk bot commented Sep 1, 2022

@gdams this pull request can not be integrated into pr/1352 due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout split
git fetch https://git.openjdk.org/jdk11u-dev pr/1352
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge pr/1352"
git push

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 1, 2022
@openjdk-notifier
Copy link

@gdams Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 1, 2022
Copy link
Contributor

@RealCLanger RealCLanger 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 now. I'll run it another night through our testing. We've had it in for the last few weeks and didn't see problems, though.

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/1352 to master September 2, 2022 07:54
@openjdk-notifier
Copy link

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout split
git fetch https://git.openjdk.org/jdk11u-dev master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

openjdk bot commented Sep 2, 2022

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

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

8239708: Split basics.m4 into basic.m4 and util.m4
8213239: Configure cannot handle command overrides with arguments
8206125: [windows] cannot pass relative path to --with-boot-jdk

Reviewed-by: clanger

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 (@RealCLanger) 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 the ready Pull request is ready to be integrated label Sep 2, 2022
@gdams
Copy link
Member Author

gdams commented Sep 2, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Sep 2, 2022

@gdams
Your change (at version 5a2c789) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 2, 2022
@RealCLanger
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 2, 2022

Going to push as commit 75b3e45.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 2, 2022
@openjdk openjdk bot closed this Sep 2, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 2, 2022
@gdams gdams deleted the split branch September 2, 2022 08:15
@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 2, 2022
@openjdk
Copy link

openjdk bot commented Sep 2, 2022

@RealCLanger @gdams Pushed as commit 75b3e45.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
2 participants