Skip to content

changelog: check more subprocess exit codes, full checkout to get tags and history#5261

Merged
lgoettgens merged 2 commits intomasterfrom
bl/updatechangelogpy
Sep 2, 2025
Merged

changelog: check more subprocess exit codes, full checkout to get tags and history#5261
lgoettgens merged 2 commits intomasterfrom
bl/updatechangelogpy

Conversation

@benlorenz
Copy link
Copy Markdown
Member

Tags and history are probably needed for merge-base command, and check might help debugging such issues.

x-ref: #5243 (comment)

@benlorenz benlorenz added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Sep 2, 2025
@benlorenz
Copy link
Copy Markdown
Member Author

benlorenz commented Sep 2, 2025

I changed the code to use HEAD instead of master when determining the merge-base since this allows running it when master doesn't exist (e.g. when I first tried running the workflow manually in CI with a custom branch).

Another option would be to use refs/remotes/origin/master which should also exist independent of the branch this is running from.

@lgoettgens
Copy link
Copy Markdown
Member

Another option would be to use refs/remotes/origin/master which should also exist independent of the branch this is running from.

This would make it harder to run from cloned forks, as there the master may be out of date, and the actual master branch is named refs/remotes/upstream/master or whatever people call their upstream remote. But HEAD should be good.

Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

I don't really understand what the scripts does nor what the changes do, but since this appears to fix the problem, I am fine with merging. Maybe @aaruni96 can do a proper review once they are back in office.

@lgoettgens lgoettgens mentioned this pull request Sep 2, 2025
16 tasks
@benlorenz
Copy link
Copy Markdown
Member Author

I don't really understand what the scripts does nor what the changes do, but since this appears to fix the problem, I am fine with merging. Maybe @aaruni96 can do a proper review once they are back in office.

The most important change is the fetch-depth: 0, otherwise actions/checkout only does a limited checkout of the repository (only the HEAD commit and no tags).
The script tries to run git merge-base v1.5dev master, but this fails because v1.5dev doesn't exist.
The check=True arguments will cause the run to throw an error in this case instead of silently continuing with probably empty output, so we at least see that is is failing and at what location. (Like this https://github.com/oscar-system/Oscar.jl/actions/runs/17401461197/job/49395382367 before I changed master to HEAD)

@lgoettgens lgoettgens added the CI label Sep 2, 2025
@lgoettgens lgoettgens merged commit 4868556 into master Sep 2, 2025
3 checks passed
@lgoettgens lgoettgens deleted the bl/updatechangelogpy branch September 2, 2025 12:03
SirToby25 pushed a commit to SirToby25/Oscar.jl that referenced this pull request Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants