Skip to content

feat: publish p2 update site to GitHub Pages via gh-pages branch#1360

Open
rubenporras wants to merge 1 commit into
dsldevkit:masterfrom
rubenporras:release_workflow
Open

feat: publish p2 update site to GitHub Pages via gh-pages branch#1360
rubenporras wants to merge 1 commit into
dsldevkit:masterfrom
rubenporras:release_workflow

Conversation

@rubenporras
Copy link
Copy Markdown
Member

  • Create a snapshot workflow that build and deploys on every push to master or v[0-9]* branches into p2/snapshots//; keep last 20, clean up older ones
  • Create a release workflow to promote existing snapshots to releases; The workflow creates a tag and copy the corresponding snapshot for that commit into p2/releases// on gh-pages
  • Add composite p2 repositories (compositeContent.xml, compositeArtifacts.xml, p2.index) for p2/releases/latest/ and p2/snapshots/latest/ so Eclipse can resolve "latest" without a fixed URL
  • Add generated index.html listing all snapshot and release links
  • Document p2 update site URLs in README.md

Copy link
Copy Markdown
Collaborator

@joaodinissf joaodinissf left a comment

Choose a reason for hiding this comment

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

Review of the publish-p2 pipeline. Overall the structure is clean — concurrency group, gh-pages bootstrap, push-with-rebase loop, pinned actions all look good. A few issues worth addressing before merge, flagged inline below.

left by Claude at João's request

Comment thread .github/workflows/release.yml Outdated

- name: Set snapshot SHA
id: snapshot
run: echo "sha=${GITHUB_SHA::8}" >> "$GITHUB_OUTPUT"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

GITHUB_SHA is not the SHA of the checked-out branch.

On workflow_dispatch, GITHUB_SHA is the SHA of the ref the workflow was dispatched on (usually master), not inputs.branch. If a user dispatches from the master selector but picks inputs.branch: release/1.0.x, the previous step checked out release/1.0.x but snapshot_sha here is master's HEAD. The verify-snapshot step then looks up the wrong snapshot — best case it 404s with a confusing error, worst case the master snapshot exists and we promote unrelated bits as release/1.0.x.

Derive from the checkout instead:

run: echo "sha=$(git rev-parse --short=8 HEAD)" >> "$GITHUB_OUTPUT"

left by Claude at João's request

Comment thread ddk-parent/pom.xml Outdated
</dependencies>
<configuration>
<timestampProvider>jgit</timestampProvider>
<jgit.ignore>pom.xml</jgit.ignore>
Copy link
Copy Markdown
Collaborator

@joaodinissf joaodinissf May 22, 2026

Choose a reason for hiding this comment

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

Correction to my earlier comment on this line — original claim was wrong. I asserted jgit.ignore was a comma-separated list of literal paths matching only the root pom.xml. Verified against tycho-extras PathFilter.java: the value is newline-split and each line is fed to JGit's FastIgnoreRule, i.e. real gitignore semantics. A bare pom.xml matches every pom.xml at any depth — exactly what one would want for an across-the-tree exemption.

With that out of the way, the substantive question is whether this exemption is correctly scoped. The default exempted set (hardcoded in PathFilter) is already .tycho-consumer-pom.xml, .polyglot.*, pom.tycho — Tycho-generated files. Adding pom.xml is a deliberate choice that says "expect human-edited pom.xml to be dirty at build time."

In the current PR, neither snapshot.yml nor release.yml modifies any pom mid-build, so the practical effect today is zero. The presumable forward-looking intent is the "automatic Bump PR" workflow alluded to in release.yml's trailing comment, which would run tycho-versions:set-version. If that's the case, the exemption is incompletetycho-versions:set-version also rewrites:

  • META-INF/MANIFEST.MF (Bundle-Version)
  • feature.xml
  • category.xml
  • *.product

so the dirty-tree check would still fire on those.

Suggested directions, in rough order of cleanliness:

  1. Commit the bump, then build. Have the bump PR's CI run tycho-versions:set-version, commit on the PR branch, then build/test from a clean tree. No jgit.ignore extension needed at all; gives a reviewable diff. This is what most Tycho projects do.
  2. If in-build bumps are genuinely required, expand jgit.ignore to cover everything tycho-versions writes (newline-separated):
    <jgit.ignore>
      pom.xml
      MANIFEST.MF
      feature.xml
      category.xml
      *.product
    </jgit.ignore>
  3. If pom.xml is here for some reason I haven't seen, an XML comment explaining the case would help future readers.

Not a blocker — just worth confirming the intended scope.

left by Claude at João's request

Comment thread ddk-parent/pom.xml
<tycho.version>5.0.2</tycho.version>
<xtend.version>2.42.0</xtend.version>

<!-- jgit qualifier: error in CI, warning for local dirty trees -->
Copy link
Copy Markdown
Collaborator

@joaodinissf joaodinissf May 22, 2026

Choose a reason for hiding this comment

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

Comment doesn't match behavior — and the lsp4e-pattern fix is concrete.

lsp4e (whose pom is clearly the template here) implements exactly what this comment promises with an !env.CI-activated profile. This PR has the comment and the error property but is missing the profile, so any local mvn verify with uncommitted changes will fail dirty.

Suggested fix for this line:

Suggested change
<!-- jgit qualifier: error in CI, warning for local dirty trees -->
<!-- jgit qualifier dirty-tree handling: error in CI; warning locally via the !env.CI profile below -->

And add this profile in the existing <profiles> section (around line 423):

<profile>
  <id>local-build</id>
  <activation>
    <property>
      <name>!env.CI</name>
    </property>
  </activation>
  <properties>
    <jgit.dirtyWorkingTree>warning</jgit.dirtyWorkingTree>
  </properties>
</profile>

GitHub Actions sets CI=true, so the profile only activates outside CI. Matches lsp4e verbatim.

left by Claude at João's request

Comment thread ddk-parent/pom.xml

<!-- Baseline version validation (skip until first release populates p2/releases/latest/) -->
<baseline.repo.url>https://dsldevkit.github.io/dsl-devkit/p2/releases/latest/</baseline.repo.url>
<baseline.skip>true</baseline.skip>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up tracking? baseline.skip=true is correct for now (no p2/releases/latest/ exists until the first release publishes), but it needs to be flipped to false once the first release lands — otherwise baseline version validation is silently disabled forever. Worth filing a follow-up issue, or wiring the property to a profile that auto-disables for the very first release only.

left by Claude at João's request

Comment thread .github/workflows/release.yml Outdated
# Snapshots: every push to master or v[0-9]* branches → see snapshot.yml
# Release: workflow_dispatch → create tag, promote snapshot to p2/releases/<version>/
# Bump: automatic PR after each release (0-4 minor cycle, then major)
# Patches: run from release/X.Y.x branch → bump PR targets release branch
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Trailing comment describes workflows that don't exist in this PR.

#   Bump:       automatic PR after each release (0-4 minor cycle, then major)
#   Patches:    run from release/X.Y.x branch → bump PR targets release branch

There's no bump-PR workflow, no release/X.Y.x patch flow. Either drop these lines or label them as "future" so reviewers/users don't go looking for them.

Also: file has no trailing newline.

left by Claude at João's request

Comment thread .github/scripts/cleanup-p2-snapshots.sh Outdated
git config user.name "github-actions[bot]"

if [ -d p2/snapshots ]; then
(cd p2/snapshots && ls -1t | grep -v latest | tail -n +21 | xargs -r -I{} rm -rf "{}")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Retention is not actually "keep the newest 20" — it's "keep the newest 1 and 19 arbitrary others".

actions/checkout does not preserve git commit times as filesystem mtimes; every file/dir on a fresh checkout gets mtime ≈ checkout time. So when this runs on a workflow with >20 existing snapshots:

  • The just-cp -r'd new snapshot has a fresh mtime (newest).
  • All previously-existing snapshot dirs share the same mtime (the checkout time), within milliseconds.
  • ls -1t ties are broken by filesystem ordering, which is undefined.

Result: on every cleanup run, which old snapshots are pruned is non-deterministic, and over time the oldest snapshots are not preferentially removed.

More reliable options:

  • Drive retention from git history: git log --diff-filter=A --name-only -- 'p2/snapshots/*' | … to find the introducing commit for each dir, sort by commit date.
  • Or stamp each new snapshot dir with a timestamp file at creation, and sort by that.
  • Or commit a small p2/snapshots/index.json listing snapshots in order, and prune from the tail.

left by Claude at João's request

@rubenporras rubenporras force-pushed the release_workflow branch 2 times, most recently from dca2f43 to 35c63dc Compare May 22, 2026 14:02
- Create a snapshot workflow that build and deploys on every push to
master or v[0-9]* branches into p2/snapshots/<sha8>/; keep last 20,
clean up older ones
- Create a release workflow to promote existing snapshots to releases;
The workflow creates a tag and  copy the corresponding snapshot for that
commit into p2/releases/<version>/ on gh-pages
- Add composite p2 repositories (compositeContent.xml,
compositeArtifacts.xml, p2.index) for p2/releases/latest/ and
p2/snapshots/latest/ so Eclipse can resolve "latest" without a fixed URL
- Add generated index.html listing all snapshot and release links
- Document p2 update site URLs in README.md

Co-authored-by: João Dinis Ferreira <Joao.Ferreira@avaloq.com>
Comment thread ddk-parent/pom.xml
</configuration>
</execution>
</executions>
</plugin>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Missing baseline-replacement plugin — compare-version-with-baselines above will false-positive on every unchanged-module rebuild without it.

The extras plugin defaults <comparator> to bytes (literal FileUtils.contentEquals). Tycho-built jars are not byte-reproducible across rebuilds out of the box: MANIFEST.MF carries Bnd-LastModified, META-INF/maven/.../pom.properties carries a build timestamp, jar entry ordering can drift, etc. So when the check finds a module at the same fully-qualified version as baseline and byte-compares, it will fail with Baseline and reactor have the same fully qualified version, but different content.

lsp4e addresses this by configuring tycho-p2-plugin with <baselineRepositories> pointing at the snapshots URL. At packaging time Tycho compares each built bundle against the baseline; where content matches semantically, the baseline jar bytes are reused wholesale. The subsequent compare-version-with-baselines byte comparison then trivially passes for unchanged modules.

Two URLs, two purposes:

  • <baselineRepositories> on tycho-p2-plugin → snapshots (byte-identical rebuilds across CI runs)
  • <baselines> on tycho-p2-extras-plugin (above) → releases (version-monotonicity check)

Suggested addition — replaces the existing </plugin> on this line with the same closing tag plus a new sibling plugin entry:

Suggested change
</plugin>
</plugin>
<plugin>
<groupId>org.eclipse.tycho</groupId>
<artifactId>tycho-p2-plugin</artifactId>
<version>${tycho.version}</version>
<configuration>
<baselineRepositories>
<repository>
<url>https://dsldevkit.github.io/dsl-devkit/p2/snapshots/latest/</url>
</repository>
</baselineRepositories>
</configuration>
</plugin>

Note: this is required only if you remove <baseline.skip>true</baseline.skip> in the follow-up; while skip is true the comparator never runs and this is dormant. But since the plan is to unskip immediately, it should land in this PR or in the same follow-up that flips the skip.

left by Claude at João's request

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