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

6889: Provide build script for making it easier to build JMC #100

Closed
wants to merge 10 commits into from

Conversation

Schalli1987
Copy link

@Schalli1987 Schalli1987 commented Jul 25, 2020

Second try, I signed the OCA last week (also I do not have any response).

This script provides an easy way to package JMC by just calling a single command.


Progress

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

Issue

  • JMC-6889: Provide build script for making it easier to build JMC

Reviewers

Download

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

Andreas Schaller added 4 commits Jul 18, 2020
@bridgekeeper bridgekeeper bot added the oca label Jul 25, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 25, 2020

Hi @Schalli1987, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Schalli1987" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@thegreystone
Copy link
Member

thegreystone commented Jul 27, 2020

Great to hear that the OCA has been signed! Sorry about all of this being a little bit involved. Good news, once you're through this part of the process, it's done for all eternity. There are a lot of vacations right now, so it may take a little bit longer than usual. Feel free to open the Skara issue if you haven't already.

@bridgekeeper bridgekeeper bot removed the oca label Aug 11, 2020
@Schalli1987
Copy link
Author

Schalli1987 commented Aug 17, 2020

Great to hear that the OCA has been signed! Sorry about all of this being a little bit involved. Good news, once you're through this part of the process, it's done for all eternity. There are a lot of vacations right now, so it may take a little bit longer than usual. Feel free to open the Skara issue if you haven't already.

OCA has now been accepted. I don't think there's a Skara issue for this right now, and it seems that I can't create one.
I need to log in but I do not have and account and there's no way to register?

@thegreystone
Copy link
Member

thegreystone commented Aug 17, 2020

Ah. If you don't have an OpenJDK identity, then that doesn't matter.

@thegreystone thegreystone changed the title Enhancement/build script 6889: Provide build script for making it easier to build JMC Aug 19, 2020
@openjdk openjdk bot added the rfr label Aug 19, 2020
@mlbridge
Copy link

mlbridge bot commented Aug 19, 2020

Webrevs

@thegreystone
Copy link
Member

thegreystone commented Aug 19, 2020

One thing that I see beginners struggling with is running the built product. Perhaps being able to launch the built product could be a target? And perhaps with -vm $JAVA_HOME/bin added?

For Mac:
target/products/org.openjdk.jmc/macosx/cocoa/x86_64/JDK\ Mission\ Control.app/Contents/MacOS/jmc -vm $JAVA_HOME/bin

For Linux:
target/products/org.openjdk.jmc/linux/gtk/x86_64/jmc -vm $JAVA_HOME/bin

@thegreystone
Copy link
Member

thegreystone commented Aug 19, 2020

We should ensure that the script works on both Mac and Linux. Check $OSTYPE. If it starts with darwin, then MacOS X.

@reinhapa
Copy link
Member

reinhapa commented Aug 19, 2020

@Schalli1987 it would also be nice to check for the existence of maven and a java runtime as precondition too...

Copy link
Member

@thegreystone thegreystone left a comment

We should ensure that the script works on both Mac and Linux. Check $OSTYPE. If it starts with darwin, then MacOS X.

One thing that I see beginners struggling with is running the built product. Perhaps adding a --run which simply launch the latest build? And perhaps launch with -vm $JAVA_HOME/bin added?

For Mac:
target/products/org.openjdk.jmc/macosx/cocoa/x86_64/JDK\ Mission\ Control.app/Contents/MacOS/jmc -vm $JAVA_HOME/bin

For Linux:
target/products/org.openjdk.jmc/linux/gtk/x86_64/jmc -vm $JAVA_HOME/bin

@thegreystone
Copy link
Member

thegreystone commented Sep 2, 2020

Hi @Schalli1987! Just wanted to check if you need any assistance with bringing this one home.

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 12, 2020

Sorry it's been a while...
Finally I added OSTYPE checks (can someone with a MAC confirm that its working?) and some preconditions for java and maven.
Also a --run option was added that can be called after --packageJmc.
Since JAVA_HOME is no longer set in JDK 11 and above, I'd use the system executable java instead.

build.sh Outdated
@@ -156,4 +188,17 @@ function parseArgs() {
done
}

function checkPreconditions() {
if ! command -v mvn &> /dev/null ; then
err_log "It seems you do not have maven installed. Please ensure you have it installend and executable as \"mvn\"."
Copy link
Member

@thegreystone thegreystone Sep 12, 2020

Choose a reason for hiding this comment

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

Typo "installend" -> "installed".

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

thegreystone commented Sep 14, 2020

Tried running on Mac:
14:06:23 building p2:site - logging output to /Users/marcus.hirt/git/myrepos/jmcreviews/Schalli1987/jmc/target/build_20200914140619.1.p2_site.log
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/Users/marcus.hirt/.m2/repository/org/codehaus/groovy/groovy-all/2.4.8/groovy-all-2.4.8.jar) to method java.lang.Object.finalize()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
14:06:33 run jetty - logging output to /Users/marcus.hirt/git/myrepos/jmcreviews/Schalli1987/jmc/target/build_20200914140619.2.jetty.log
ps: illegal option -- -
usage: ps [-AaCcEefhjlMmrSTvwXx] [-O fmt | -o fmt] [-G gid[,gid...]]
[-g grp[,grp...]] [-u [uid,uid...]]
[-p pid[,pid...]] [-t tty[,tty...]] [-U user[,user...]]
ps [-L]

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 15, 2020

Which version of ps do you use?
I have

$ ps --version
ps from procps-ng 3.3.16

The --ppid option was added in 3.1.6 back in 2003?

@thegreystone
Copy link
Member

thegreystone commented Sep 16, 2020

Which version of ps do you use?

This is the Mac version of ps, which seems to have slightly different parameter set. I'm running on Catalina 10.15.6.

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 16, 2020

Tried another way to terminate jetty server, please check again.

@thegreystone
Copy link
Member

thegreystone commented Sep 17, 2020

Hi @Schalli1987 - it will now wait indefinitely for the jetty server to start:

HIRTMAC:jmc marcus.hirt$ ./build.sh --packageJmc
10:36:42 building p2:site - logging output to /Users/marcus.hirt/git/myrepos/jmcreviews/Schalli1987/jmc/target/build_20200917103639.1.p2_site.log
10:36:53 run jetty - logging output to /Users/marcus.hirt/git/myrepos/jmcreviews/Schalli1987/jmc/target/build_20200917103639.2.jetty.log
10:36:53 waiting for jetty server to start
10:36:55 waiting for jetty server to start
10:36:56 waiting for jetty server to start
10:36:57 waiting for jetty server to start
10:36:58 waiting for jetty server to start
10:36:59 waiting for jetty server to start
10:37:00 waiting for jetty server to start
10:37:01 waiting for jetty server to start
10:37:02 waiting for jetty server to start
10:37:03 waiting for jetty server to start

It seems to be because after the line [INFO] Started Jetty Server, I also get these:
[INFO] Using Non-Native Java sun.nio.fs.PollingWatchService
[WARNING] Quiet Time is too low for non-native WatchService [sun.nio.fs.PollingWatchService]: 1000 < 5000 ms (defaulting to 5000 ms)

Also - currently JMC requires Java 8 to build. Perhaps java -version could be checked to ensure that it is JDK 8? Otherwise you get a whole bunch of cryptic illegal reflective access warnings:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/Users/marcus.hirt/.m2/repository/org/codehaus/groovy/groovy-all/2.4.8/groovy-all-2.4.8.jar) to method java.lang.Object.finalize()
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.reflection.CachedClass
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Exiting with a message about switching to JDK 8 would be friendlier.

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 17, 2020

You're right, I didn't expect to be more output there, changed to check entire file.
I'm building and running JMC using JDK 11 without any issues, so I think there should be no enforcement for downgrading to JDK 8.
The illegal reflective access warnings shouldn't bother here.

Copy link
Member

@thegreystone thegreystone left a comment

Just a small typo left, then we're good to go! :)

build.sh Outdated
exit 1
}

echo "$(date +%T) installing core artfacts - logging output to ${installLog}"
Copy link
Member

@thegreystone thegreystone Sep 17, 2020

Choose a reason for hiding this comment

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

Typo. artfacts -> artifacts.

@openjdk openjdk bot removed the rfr label Sep 18, 2020
@thegreystone
Copy link
Member

thegreystone commented Sep 18, 2020

JCheck seems to be failing:
"⚠️ For contributors who are not existing OpenJDK Authors, commit attribution will be taken from the commits in the PR. However, the commits in this PR have inconsistent user names and/or email addresses. Please amend the commits."

@thegreystone
Copy link
Member

thegreystone commented Sep 18, 2020

Ugh:
Last commit:
Author: Schalli1987 github@schal.li
vs. other commits:
Author: Andreas Schaller schalli1987@gmail.com

@thegreystone
Copy link
Member

thegreystone commented Sep 18, 2020

@Schalli1987 - can you force push an amend?

@openjdk
Copy link

openjdk bot commented Sep 19, 2020

@Schalli1987 This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

6889: Provide build script for making it easier to build JMC

Reviewed-by: hirt
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 11 commits pushed to the master branch:

  • 16513ca: 6882: Method Profiling lacks icon in the Java Application View
  • ea46b65: 6487: The TOPIC word in topic constants should either be the first word or removed
  • 9cac6ab: 6902: Console plug-ins with icons not found will fail
  • 15f6cca: 6897: Remove unused javax imports
  • d0be216: 6898: Installing from IDE update site complains about missing LZ4 plug-in
  • db64b84: 6000: Get rid of build warnings in the JMC agent
  • 76d656c: 6887: Agent instrumentation fails silently if method descriptors don't match
  • f157e54: 6875: Add agent MXBean operation to return instrumented xml configuration
  • 3aa92e4: 6870: defineEventProbes is incorrect when probes occupy different classes
  • 38f9835: 6885: VM_OPERATION_DURATION aggregator does not filter on type
  • ... and 1 more: https://git.openjdk.java.net/jmc/compare/22758a7be462468c4dcd3f150b49363d739e6716...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 16513ca16b1421c443c22ce2146819fddb8258c7.

As you do not have Committer status in this projectan existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone) 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).

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 19, 2020

Done 😅

@Schalli1987
Copy link
Author

Schalli1987 commented Sep 19, 2020

/integrate

@openjdk openjdk bot added the sponsor label Sep 19, 2020
@openjdk
Copy link

openjdk bot commented Sep 19, 2020

@Schalli1987
Your change (at version deb105d) is now ready to be sponsored by a Committer.

@thegreystone
Copy link
Member

thegreystone commented Sep 19, 2020

/sponsor

@openjdk openjdk bot closed this Sep 19, 2020
@openjdk
Copy link

openjdk bot commented Sep 19, 2020

@thegreystone @Schalli1987 Since your change was applied there have been 11 commits pushed to the master branch:

  • 16513ca: 6882: Method Profiling lacks icon in the Java Application View
  • ea46b65: 6487: The TOPIC word in topic constants should either be the first word or removed
  • 9cac6ab: 6902: Console plug-ins with icons not found will fail
  • 15f6cca: 6897: Remove unused javax imports
  • d0be216: 6898: Installing from IDE update site complains about missing LZ4 plug-in
  • db64b84: 6000: Get rid of build warnings in the JMC agent
  • 76d656c: 6887: Agent instrumentation fails silently if method descriptors don't match
  • f157e54: 6875: Add agent MXBean operation to return instrumented xml configuration
  • 3aa92e4: 6870: defineEventProbes is incorrect when probes occupy different classes
  • 38f9835: 6885: VM_OPERATION_DURATION aggregator does not filter on type
  • ... and 1 more: https://git.openjdk.java.net/jmc/compare/22758a7be462468c4dcd3f150b49363d739e6716...master

Your commit was automatically rebased without conflicts.

Pushed as commit f17d3a9.

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

@openjdk openjdk bot removed the rfr label Sep 19, 2020
@thegreystone
Copy link
Member

thegreystone commented Sep 19, 2020

Thank you @Schalli1987 for your contribution! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants