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
JDK-8257547: Handle multiple prereqs on the same line in deps files #1548
Conversation
|
Webrevs
|
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.
As before, regex is write-only code. :-& But I think this does what you claim it to do. :)
I'm not sure how hard it would be to add unit testing of this function (probably quite tricky), but it's starting to get close to the point were it's worth it.
@erikj79 This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 28 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
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.
As @magicus wrote - this is subtle but looks OKAY.
I added a test as suggested. This uncovered some portability issues as the test failed on Macosx. I have modified the fix-deps-file macro even more to make it portable for all the versions of sed we are currently using. |
$(ECHO) " $(WORKSPACE_ROOT)/bar \\" >> $(DEPS_FILE).expected | ||
$(ECHO) " $(WORKSPACE_ROOT)/bar/baz \\" >> $(DEPS_FILE).expected | ||
$(ECHO) " /foo/baz" >> $(DEPS_FILE).expected | ||
$(DIFF) $(DEPS_FILE).expected $(DEPS_FILE) |
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.
Does this need to be:
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.
No, the fix-deps-file macro takes file.tmp as input and outputs into file, so $(DEPS_FILE) is the output file from the macro in this 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.
OK
/integrate |
@erikj79 Since your change was applied there have been 53 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 36209b7. |
After fixing JDK-8256810 and starting to look into backporting it, I discovered more potential failing cases. Certain versions of GCC may sometimes output multiple prerequisite files on the same line. I think the easiest way to handle this new issue is to split such lines.
When splitting lines, we need to make sure to not just split on any space. Some compilers output the : of the rule with a leading space, and already split lines will have a space before the backslash.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1548/head:pull/1548
$ git checkout pull/1548