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

CI: when getting pharo version/prefix on PR, do not use --first-parent #11780

Merged
merged 1 commit into from Oct 19, 2022

Conversation

privat
Copy link
Contributor

@privat privat commented Oct 11, 2022

Current CI bash scripts use git describe --first-parent to extract version information from the git repository. It is used cosmetically to feed values to versions numbers, but is also used to enable/disable some tests according to the version number.

Instead of using the merge commits automatically done by github for each PR, Jenkins is doing its own merge commit with the PR head and the target branch. Unfortunately, Jenkins does it in the wrong order, where the PR is the first parent and the target branch is the second one.

While the order of parents on merge commits have a very little influence on git (and this is good!), there is an actual (opt-in) tradition of the first parent being the main branch and other parents being feature branches. This tradition gives, for instance, the option --first-parent of the git describe command.

All this made that old commits merged on newer branches will use the version information of these older commits instead of the correct more recent one, thus making some tests failing. Eg #11770 and #11771

Note: Since this is a CI related PR, and that there is no meta-CI infrastructure, there is no easy way to see if this really solve the issue of the aforementioned PR. So, in order to test the effects, this PR is artificially based on a quite old pharo commit.

@privat
Copy link
Contributor Author

privat commented Oct 11, 2022

opps, I used a base that was too old, so the fix was made ineffective.
Retry with v10.0.0 as a base.

@privat
Copy link
Contributor Author

privat commented Oct 11, 2022

This seems to work, as we can see in https://ci.inria.fr/pharo-ci-jenkins2/blue/rest/organizations/jenkins/pipelines/Test%20pending%20pull%20request%20and%20branch%20Pipeline/branches/PR-11780/runs/3/nodes/20/log/?start=0 that the CI scripts set the version to 11.0 (that is what this PR expected to have)

@marcor
Copy link
Contributor

marcor commented Oct 11, 2022

Ah thanks! I think I see now why adding a version tag in my PR didn't do the trick. It was behind the merge commits.

Does this behavior of Jenkins also explain why the console output reads:

"Merging remotes/origin/Pharo11 commit 89c278[...] into PR head commit 560265[...]"

instead of "merging PR commit into origin"?

So in theory after this PR is merged in Pharo11, my PR #11771 should also pass the tests, right?

@privat
Copy link
Contributor Author

privat commented Oct 11, 2022

Yes, it is my bet that once this PR (and its sibling #11785) is merged, the PR #11771 (and respectively its sibling #11770) should pass the tests (or, at least, pass more tests) if you retry the jenkins

@MarcusDenker
Copy link
Member

What now is strange: we have a test failing:

SpJobListPresenterTest>>#testJobIsFinishedWhenWaitingMoreThanWorkBlockDuration

but this is tagged in Pharo11 as #skipOnPharoCITestingEnvironment (in Pharo10 not, so there it failing makes sense).

Looking at the build log, it seems to builds a Pharo10 image.

@privat
Copy link
Contributor Author

privat commented Oct 12, 2022

Damn, you are right. :(
I will look at that.

@privat
Copy link
Contributor Author

privat commented Oct 12, 2022

Ok, I have an explication: there is a bug on the Jenkins binding with Github, as the result presented is this page https://ci.inria.fr/pharo-ci-jenkins2/job/Test%20pending%20pull%20request%20and%20branch%20Pipeline/job/PR-11785/1/display/redirect is the one of PR #11785 for pharo10. I may assume that since the head commit is the same on both PR, there is some bad confusion somewhere.

The correct Jenkins result page for this PR is https://ci.inria.fr/pharo-ci-jenkins2/blue/organizations/jenkins/Test%20pending%20pull%20request%20and%20branch%20Pipeline/detail/PR-11780/3/pipeline that is super green!

@marcor
Copy link
Contributor

marcor commented Oct 12, 2022

Yes it happened for #11770 as well, it seems that if multiple PRs point to the same commit only the latest is picked by the GitHub interface.

@MarcusDenker MarcusDenker merged commit 89225f6 into pharo-project:Pharo11 Oct 19, 2022
@MarcusDenker
Copy link
Member

Let's try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants