-
Notifications
You must be signed in to change notification settings - Fork 89
7904122: Compilation of jasm files leads to class file version warning #303
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
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
|
@jaikiran This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
| $(GREP) '\---jasm-stderr:' $(@:%.ok=%)/work/JasmUsageTest.jtr > /dev/null | ||
| $(GREP) '\---jasm:' $(@:%.ok=%)/work/JasmUsageTest.jtr > /dev/null | ||
| # verify that the warning wasn't printed | ||
| $(GREP) 'Warning: Class file version not specified in file or by -cv parameter' \ |
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.
Would it make sense to (also) look for any other "Warning: ..." messages? 🤔
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.
Hello Christian, I wouldn't expect any other warning message and if there is one, I think we shouldn't allow that warning to fail this test. This test is merely interested in verifying that the class file version check warning message is not logged.
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.
Okay.
Yet, with such a broader assertion, we would have caught the issue with compiling .jasm files earlier.
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.
We would have to narrow down the "grep" to specific lines in that jtr file to be sure that only warning(s) from the @compile <file>.jasm action are considered in that check. That will require some grep skills. I can experiment with it, if you prefer we check for other warnings too.
Can I please get a review of this change which fixes the issue noted in https://bugs.openjdk.org/browse/CODETOOLS-7904122?
The commit here adds
-cv 52.0as an option when launching thejasmtool for compilingjasmfiles through jtreg's@compileaction. This class file version will be used as the default version if the jasm file doesn't explicitly specify a class file version in it. The use of "-cv" option prevents the warning from being issued.An existing self test has been updated to verify this change.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jtreg.git pull/303/head:pull/303$ git checkout pull/303Update a local copy of the PR:
$ git checkout pull/303$ git pull https://git.openjdk.org/jtreg.git pull/303/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 303View PR using the GUI difftool:
$ git pr show -t 303Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jtreg/pull/303.diff
Using Webrev
Link to Webrev Comment