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

6946: There should be an option to build the agent in the build script #183

Conversation

mirage22
Copy link
Collaborator

@mirage22 mirage22 commented Dec 14, 2020

https://bugs.openjdk.java.net/browse/JMC-6946
added:

  • build.sh --packageAgent
  • build.sh --runAgentExample

Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-6946: There should be an option to build the agent in the build script

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jmc pull/183/head:pull/183
$ git checkout pull/183

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 14, 2020

👋 Welcome back mwengner! 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 added the rfr label Dec 14, 2020
build.sh Outdated Show resolved Hide resolved
Copy link
Member

@thegreystone thegreystone left a comment

Don't forget about windows!

@mirage22
Copy link
Collaborator Author

mirage22 commented Dec 14, 2020

seem the whole script was since the beginning not windows ready. would not be better to create a new ticket ? As this is not really related to this one, what do you think ?

@jpbempel
Copy link
Member

jpbempel commented Dec 15, 2020

seem the whole script was since the beginning not windows ready. would not be better to create a new ticket ? As this is not really related to this one, what do you think ?

There is build.bat script for windows

build.sh Show resolved Hide resolved
build.sh Show resolved Hide resolved
@mirage22
Copy link
Collaborator Author

mirage22 commented Dec 16, 2020

I've updated the win script, but we need to test it on windows machine. I don't have any. I've not found the simple way of parsing the java version to run different types of examples, therefore I've omitted such option for the win sritpt.

@mirage22
Copy link
Collaborator Author

mirage22 commented Dec 16, 2020

I've add an option "--installCore" for both

@reinhapa
Copy link
Member

reinhapa commented Dec 17, 2020

Why was the Jetty start removed from the --test and --package option? In case of calling those tasks with not previously calling --build it will lead to errors as is did not before.

@mirage22
Copy link
Collaborator Author

mirage22 commented Dec 17, 2020

Why was the Jetty start removed from the --test and --package option? In case of calling those tasks with not previously calling --build it will lead to errors as is did not before.
good question: as those option I've not modified, I see runTests -> --test(corrected) . there is no option --package, there is an option --packageJmc and it executes properly startJetty function

@mirage22 mirage22 requested review from thegreystone and aptmac Dec 30, 2020
build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
build.sh Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
@mirage22
Copy link
Collaborator Author

mirage22 commented Jan 12, 2021

I've corrected unrecognized java version

build.sh Outdated Show resolved Hide resolved
Copy link
Member

@aptmac aptmac left a comment

Just a couple of minor nits, the functionality seems fine to me.

build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
Copy link
Member

@aptmac aptmac left a comment

I found a nifty dictionary vscode extension that pointed out a couple last ones, should be good after this.

build.sh Outdated Show resolved Hide resolved
build.sh Outdated Show resolved Hide resolved
@mirage22
Copy link
Collaborator Author

mirage22 commented Jan 13, 2021

hopefully all typos corrected

build.sh Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Jan 13, 2021

@mirage22 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:

6946: There should be an option to build the agent in the build script

Reviewed-by: hirt, aptmac

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 6 new commits pushed to the master branch:

  • b6fe7ca: 7002: Too large graphs will freeze JMC
  • deb32e7: 7081: Update Copyright year to 2021, Validate and update license attributes
  • 549cba0: 7079: Numeric values in automated analysis summary is not resolved
  • 1e7cd87: 7076: Rules and Analysis code has exceptions during uitests
  • a21771c: 7032: The banner is missing from help page
  • 8d34bbb: 6973: New Method Profiling Page for JMC

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ 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 label Jan 13, 2021
@thegreystone thegreystone requested a review from aptmac Jan 13, 2021
@thegreystone
Copy link
Member

thegreystone commented Jan 13, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jan 13, 2021

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

@openjdk openjdk bot removed the ready label Jan 13, 2021
aptmac
aptmac approved these changes Jan 13, 2021
Copy link
Member

@aptmac aptmac left a comment

lgtm, thanks for making the changes

@openjdk openjdk bot added the ready label Jan 13, 2021
@thegreystone
Copy link
Member

thegreystone commented Jan 14, 2021

You're good to go @mirage22.

@mirage22
Copy link
Collaborator Author

mirage22 commented Jan 14, 2021

/integrate

@openjdk openjdk bot closed this Jan 14, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 14, 2021
@openjdk
Copy link

openjdk bot commented Jan 14, 2021

@mirage22 Since your change was applied there have been 7 commits pushed to the master branch:

  • 8d6fb1b: 7063: JfrThreadsPageTest (UI-test) fails
  • b6fe7ca: 7002: Too large graphs will freeze JMC
  • deb32e7: 7081: Update Copyright year to 2021, Validate and update license attributes
  • 549cba0: 7079: Numeric values in automated analysis summary is not resolved
  • 1e7cd87: 7076: Rules and Analysis code has exceptions during uitests
  • a21771c: 7032: The banner is missing from help page
  • 8d34bbb: 6973: New Method Profiling Page for JMC

Your commit was automatically rebased without conflicts.

Pushed as commit 33e684b.

💡 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
5 participants