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

Automate _ext project version bumps #1054

Closed
wants to merge 7 commits into from
Closed

Automate _ext project version bumps #1054

wants to merge 7 commits into from

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Dec 29, 2021

  • Fix _ext project build problems introduced with JUnit5
  • Store dependency locks for releases
  • Provide helper task to create lib-extra lock files
  • Provide documentation

The original idea was to update _ext projects seldomly, assuming that more Eclipse plugins will be published via M2.
The aim of this PR is:

  • Mere version bumps for _ext projects including P2 classes (all, except JDT) should be accomplished by updating properties and doing the build/publish on CI. Only if CI release fails, the P2 repo will be downloaded and a manual check will be accomplished
  • Documentation and helper tasks to encourage other developers to perform version upgrades for Eclipse plugins

Note: The new generateLibExtraLockFile uses a unified lib-extra resource structure. Unfortunately the current structure has a few flaws, hence the generated files are not stored in the "correct" location. The problem will be solved by a lib-extra resource structure clean-up in a future PR.

- Fix _ext project build problems introduced with JUnit5
- Store dependency locks for releases
- Provide helper task to create lib-extra lock files
- Provide documentation
@fvgh fvgh requested a review from nedtwigg December 29, 2021 18:21
@fvgh
Copy link
Member Author

fvgh commented Dec 29, 2021

@nedtwigg , I need your help...

  1. Upgraded to CDT version 2021-12 / 10.5. #1030 passed the CI build. CI should have failed, since JUnit5 should have complained about missing engine. Reason is the source-set I need to build manually. Furthermore I found that for WTP the test set was empty, because the WTP generates its own test configuration per type (CSS, HTML, ...). That configuration did not specify JUnit5, so no test is executed. Could you tell me, where I can see the test results in the CI?
  2. Please have a look how I integrated generateLibExtraLockFile into the spotless-changelog live cycle. I did some tests and it seems to work, but you have more experience...

@nedtwigg
Copy link
Member

Very cool!

CI should have failed, since JUnit5 should have complained about missing engine.

I assume you mean tests in _ext/eclipse-cdt should have failed? e.g.

https://github.com/diffplug/spotless/blob/main/_ext/eclipse-cdt/src/test/java/com/diffplug/spotless/extra/eclipse/cdt/EclipseCdtFormatterStepImplTest.java

They are not run on CI. My understanding is that we can't really run them until the published artifact is available on mavenCentral, and there's a delay from when we do the publish to when the artifact is actually available.

If we want the CI tests to run before we publish (reasonable), then I think we'd need to figure out some way to generate the fat jar, make the test task depend on the fat jar task, and pass the location of the fat jar as a system property so that it can be used inside the integration test.

@fvgh
Copy link
Member Author

fvgh commented Dec 30, 2021

If we want the CI tests to run before we publish (reasonable), then I think we'd need to figure out some way to generate the fat jar

You can already trigger the tests.
The tests depending on the fat jar using a custom runtimeClasspath. I missed to include the source runtime class path (only added compile class path). The JUnit5 jupiter artifact uses runtime dependency for the engine, so these tests failed since #900 (sorry, did not spot the problem during review, had no time for proper testing). This PR fixes these problems.

My understanding is that we can't really run them until the published artifact is available on mavenCentral

It is possible that you experienced build problems before. When I introduced the locking files, we used a gradle version which was not supporting that beta feature. We use the locking files in the first place, since our Eclipse glue-code uses internal interfaces of Eclipse, meaning that each Eclipse version change can break our code. Since the _ext project build process did not use locked dependencies, the build was able to break any time a new version of a Eclipse dependency was released on Maven Central. This PR fixes that problem as well.

To summarize: This PR makes the _ext project builds deterministic and fixes the test tasks.

TODO:
I am not so familiar with the spotless-changelog build cycle. Can you provide the missing changes to trigger _ext projects tests by the CI jobs? Or do we need a custom dependency between changelogCheck and test in the gradle scripts? I am surprised that the changelogCheck does not depend on a successful build by default.

The goal is to build&test simple Eclipse version bumps on the CI, so that the developer is not forced to checkout the P2 repos all the time, build the _ext project locally, ....
This also means that the CI job may fail in the "seldom" cases where code adaptation is required. So the developer would want to check the build on a feature branch. If I try to test "deploy" on a feature branch, the changelogCheck already fails the CI job before compilation & testing, because I am not on the main branch, wouldn't it? And I also think there are some restrictions on CI preventing to trigger the deploy task on a feature branch. Any suggestions to work-around?

@nedtwigg
Copy link
Member

nedtwigg commented Jan 5, 2022

I think we want a new Gradle plugin like this.

// settings.gradle
if (getStartProperty('com.diffplug.spotless.include.ext') == 'ifgitdiff') {
  file('_ext').eachDirMatch(~/^(?!(\.|gradle)).*/) { dir ->
    ifGitDiff {
      folder dir, {
        include dir.name
        project(":${dir.name}").projectDir = dir
      }
    }
  }
}

Then we can get rid of the com.diffplug.spotless.include.ext.nop2 mechanism, because only the necessary projects will be included.

We can change ext_changelog_print to ext_test:

// in every _ext project
tasks.register('ext_test') {
  dependsOn 'test'
}
// on CI
./gradlew -Pcom.diffplug.spotless.include.ext=ifgitdiff ext_test
./gradlew -Pcom.diffplug.spotless.include.ext=ifgitdiff changelogPrint

Does this sound okay to you @fvgh? I really like your idea to get the CI running on the _ext projects to improve quality and reduce dev time.

- com.diffplug.spotless.include.ext.nop2 is gone
- com.diffplug.spotless.include.ext=ifgitdiff means configure ext projects which changed since origin/ifgitdiff
- changelogPrint, changelogPush, and test all have `ext_` prefixed tasks for just the ext projects
@nedtwigg
Copy link
Member

nedtwigg commented Jan 7, 2022

I implemented the idea above, with a few tweaks:

  • CI now runs ext_test on every commit. That means that it will run test for every _ext project which has changed since origin/ifgitdiff, which is currently set to the most recent ext publish.
  • On the main branch, CI runs ext_changelog_print will print the next version for every project which has changed since origin/ifgitdiff. Instead of 5 different release tasks, there is now just one ext_release, which will release every project shown by ext_changelog_print

This means that:

  1. The test tasks is run for any PR which changes any _ext project
  2. p2asmaven only runs for the projects which have been changed
  3. once the PR gets merged, ext_release will release whatever was changed, and then move origin/ifgitdiff to that point

You can see that CI is currently failing, but locally I get the same failure before my commits, so I think that means it's working :)

@nedtwigg nedtwigg changed the base branch from main-temp to main April 20, 2022 21:31
@nedtwigg
Copy link
Member

I think all builds are working now, with easy publishing, except for the WTP, where the failures in EclipseCssFormatterStepImplTest look real to me. We could delete WTP for now and just get the others up to date?

@nedtwigg
Copy link
Member

@nedtwigg nedtwigg closed this Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants