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

8031567: Better model for storing source revision information #62

Closed
wants to merge 11 commits into from

Conversation

gnu-andrew
Copy link
Member

@gnu-andrew gnu-andrew commented May 23, 2022

In implementing JDK-8222975 and backporting JDK-8210283, we have avoided backporting JDK-8031567 in order to keep things simple. However, the net result has been that we have introduced unique 8u bugs

Rather than just patching up 8u again, it seems to make sense to just bring it into line with later JDKs. There might have been a reason to keep 8u different when it was a forest of Mercurial repositories, but it is now a single Git tree like the other JDK repositories. Handling the same thing but in a unique way just makes any future backporting harder to deal with.

In light of this, I've now backported 8031567. I started by reverting the two 8u-specific patches, 8210283 & 8222975, and then applied 8136771 & 8031567 (which includes 8222975), along with follow-up fixes 8170385 & 8170392. 8210283 was then rebackported on top of this.

A couple of additional fixes were then made to workaround Makefile differences in 8u. Due to the lack of JDK-8069261 the MakeDir usage needs to be wrapped in eval. 8u also doesn't have the modern macro format from JDK-8074988 and earlier fixes, so the SetupGetRevisionForRepo macro was modified to work in a similar way to other macros do in 8u. The stripping of $1 is necessary to avoid it trying to create variables beginning with a space (something which led to empty variables and confused me for some time)

I did consider backporting those as well, but felt there were too intrusive to other areas of the build. At least we now have a template in this patch for how to convert any future instances.

The end result actually looks pretty straightforward and differences between this and the make/SourceRevision.gmk file in 11u are small.


Progress

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

Issues

  • JDK-8031567: Better model for storing source revision information
  • JDK-8170385: JDK-8031567 broke source bundles
  • JDK-8170392: JDK-8031567 broke builds from source bundles

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk8u-dev pull/62/head:pull/62
$ git checkout pull/62

Update a local copy of the PR:
$ git checkout pull/62
$ git pull https://git.openjdk.java.net/jdk8u-dev pull/62/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 62

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk8u-dev/pull/62.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 23, 2022

👋 Welcome back andrew! 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 27b7ab8b27a5548ed4cd823d35c8190a594bfdd1 8031567: Better model for storing source revision information May 23, 2022
@openjdk
Copy link

openjdk bot commented May 23, 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 May 23, 2022
@mlbridge
Copy link

mlbridge bot commented May 23, 2022

Webrevs

@gnu-andrew
Copy link
Member Author

/issue add 8136771,80170385,8170392

@openjdk
Copy link

openjdk bot commented May 23, 2022

@gnu-andrew The issue 8136771 was not found in the JDK project - make sure you have entered it correctly.
The issue 80170385 was not found in the JDK project - make sure you have entered it correctly.
As there were validation problems, no additional issues will be added to the list of solved issues.

@gnu-andrew
Copy link
Member Author

/issue add 8170385,8170392

@gnu-andrew
Copy link
Member Author

Looks like 8136771 is private.

@openjdk
Copy link

openjdk bot commented May 23, 2022

@gnu-andrew
Adding additional issue to issue list: 8170385: JDK-8031567 broke source bundles.

Adding additional issue to issue list: 8170392: JDK-8031567 broke builds from source bundles.

@gnu-andrew
Copy link
Member Author

Note that this PR also updates common/autoconf/generated-configure.sh which was missed by #33

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

This looks OK to me. I've tested this with an tar archive as sources and works fine. Same with git.

@shiyuexw If you could test this too, I'd appreciate it. Thanks!

@jerboaa
Copy link
Contributor

jerboaa commented May 24, 2022

@gnu-andrew Could you please merge with master please, then mac os x builds should be happy too.

@openjdk
Copy link

openjdk bot commented May 24, 2022

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

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

8031567: Better model for storing source revision information
8170385: JDK-8031567 broke source bundles
8170392: JDK-8031567 broke builds from source bundles

Reviewed-by: sgehwolf

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 24, 2022
@shiyuexw
Copy link

This looks OK to me. I've tested this with an tar archive as sources and works fine. Same with git.

@shiyuexw If you could test this too, I'd appreciate it. Thanks!

I have verified it. It can work now.

@gnu-andrew
Copy link
Member Author

Rebased. That should hopefully fix the Windows weirdness too.

@gnu-andrew
Copy link
Member Author

@jerboaa I've flagged the bug with jdk8u-fix-request

@gnu-andrew
Copy link
Member Author

I see jdk8u-fix-yes.

@gnu-andrew
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 26, 2022

Going to push as commit e3b9a06.

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

openjdk bot commented May 26, 2022

@gnu-andrew Pushed as commit e3b9a06.

💡 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
Development

Successfully merging this pull request may close these issues.

3 participants