jmc icon indicating copy to clipboard operation
jmc copied to clipboard

7576: Remove the need for a local jetty server for dependencies

Open abrooksv opened this issue 3 years ago • 5 comments
trafficstars

Using newer Tycho features to achieve result:

Maven dependencies in a target (Tycho 2.2 ): https://github.com/eclipse/tycho/blob/master/RELEASE_NOTES.md#support-for-m2e-pde-maven-target-locations Nested target to cut down on duplicate third party code (Tycho 2.6): https://github.com/eclipse/tycho/blob/master/RELEASE_NOTES.md#support-for-nested-targets

Note: Due to change in 2.7, you must delete a file when switching branches due to changes in the .m2 folder: https://github.com/eclipse/tycho/blob/master/RELEASE_NOTES.md#caution-when-switching-between-tycho-versions


Progress

  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

  • JMC-7576: Remove the need for a local jetty server for dependencies

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc pull/387/head:pull/387
$ git checkout pull/387

Update a local copy of the PR:
$ git checkout pull/387
$ git pull https://git.openjdk.org/jmc pull/387/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 387

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/387.diff

abrooksv avatar Mar 18 '22 21:03 abrooksv

Hi @abrooksv, 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 abrooksv" 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.

bridgekeeper[bot] avatar Mar 18 '22 21:03 bridgekeeper[bot]

/covered

abrooksv avatar Mar 21 '22 14:03 abrooksv

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

bridgekeeper[bot] avatar Mar 21 '22 14:03 bridgekeeper[bot]

Webrevs

mlbridge[bot] avatar Mar 30 '22 23:03 mlbridge[bot]

Update for this PR: Eclipse can't resolve the third party dependencies in the manifest. This seems to be a known PDE issue: https://stackoverflow.com/questions/13728801/why-cant-manifest-mf-see-packages-from-mavens-pom

thegreystone avatar Apr 05 '22 14:04 thegreystone

@abrooksv this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout removeJetty
git fetch https://git.openjdk.org/jmc master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Feb 17 '23 18:02 openjdk[bot]

Hi @abrooksv! Is this something you'd like to get into JMC 9? Do you know if your changes will work well in the PDE / Eclipse?

thegreystone avatar Oct 25 '23 17:10 thegreystone

@abrooksv This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Dec 05 '23 01:12 bridgekeeper[bot]

Hey @thegreystone, it was rough around the edges in Eclipse at the time, but PDE and Tycho were also a moving target in their improvements in the area.

I am not sure the state of it now since I have not used Eclipse in over a year

abrooksv avatar Dec 06 '23 19:12 abrooksv

Hi Austin! Want to give this one last shot before JMC 9? We're in ramp down now, but I believe we could take this one if finished up.

thegreystone avatar Jan 03 '24 18:01 thegreystone

@abrooksv Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

openjdk[bot] avatar Jan 04 '24 02:01 openjdk[bot]

Hey Marcus!

I started over and now it is happy inside of Eclipse. I only sanity tested the latest target so far but there is a small hitch. Now that Tycho is speaking Maven coords, it seems to dislike that we defined Jetty to be 10.0.17 while Eclipse Platform is shipping 10.0.15 which was upgraded in #532. It seems it wants to to match:

[ERROR] Failed to execute goal org.eclipse.tycho:target-platform-configuration:3.0.4:target-platform (default-target-platform) on project org.openjdk.jmc.feature.rcp: Execution default-target-platform of goal org.eclipse.tycho:target-platform-configuration:3.0.4:target-platform failed: The Maven artifact to be added to the target platform is not stored at the required location on disk: required "/Users/austin.brooks/.m2/repository/org/eclipse/jetty/jetty-io/10.0.15/jetty-io-10.0.15.jar" but was "/Users/austin.brooks/.m2/repository/org/eclipse/jetty/jetty-io/10.0.17/jetty-io-10.0.17.jar" -> [Help 1]

This limitation may cause issues to arise in the other platforms I assume, if so we would probably need to match the Jetty webcsocket version to the version of the Eclipse platform of the target platform (probably something that should have always been done to avoid risk of runtime error?)

Also means we will need to inline the third party libs again vs using a shared file, but that's just a return to the old way.

abrooksv avatar Jan 04 '24 03:01 abrooksv

@abrooksv This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Feb 01 '24 06:02 bridgekeeper[bot]

@abrooksv This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar Feb 29 '24 10:02 bridgekeeper[bot]