Skip to content

ci: Fix merge_script in MSVC task #22912

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 1 commit into from
Sep 9, 2021
Merged

ci: Fix merge_script in MSVC task #22912

fanquake merged 1 commit into from
Sep 9, 2021

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 7, 2021 •
edited
Loading

The new merge_script in the MSVC build task does not really exit early when the task is triggered by a non-pr.

In the current code

bitcoin/.cirrus.yml

Line 104 in e4aa9b1

- ps: if ($env:CIRRUS_PR -eq $null) { exit 0; }

the exit 0 command exits from the PowerShell call, not the recent merge_script. This cause the next lines

bitcoin/.cirrus.yml

Lines 105 to 107 in e4aa9b1

- ps: git fetch $env:CIRRUS_REPO_CLONE_URL $env:CIRRUS_BASE_BRANCH
# Merge base to detect silent merge conflicts.
- ps: git merge FETCH_HEAD
are executed unconditionally.

Here is an excerpt from CI task log for the "Merge #22915: Remove confusing CAddrDB " commit:

...
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call powershell.exe -NoLogo -EncodedCommand ZwBpAHQAIAByAGUAcwBlAHQAIAAtAC0AaABhAHIAZAA= 
HEAD is now at 896649996 Merge bitcoin/bitcoin#22915: Remove confusing CAddrDB

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>if 0 NEQ 0 exit /b 0 

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call powershell.exe -NoLogo -EncodedCommand aQBmACAAKAAkAGUAbgB2ADoAQwBJAFIAUgBVAFMAXwBQAFIAIAAtAGUAcQAgACQAbgB1AGwAbAApACAAewAgAGUAeABpAHQAIAAwADsAIAB9AA== 

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>if 0 NEQ 0 exit /b 0 

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call powershell.exe -NoLogo -EncodedCommand ZwBpAHQAIABmAGUAdABjAGgAIAAkAGUAbgB2ADoAQwBJAFIAUgBVAFMAXwBSAEUAUABPAF8AQwBMAE8ATgBFAF8AVQBSAEwAIAAkAGUAbgB2ADoAQwBJAFIAUgBVAFMAXwBCAEEAUwBFAF8AQgBSAEEATgBDAEgA 
From https://github.com/bitcoin/bitcoin
 * branch                HEAD       -> FETCH_HEAD

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>if 0 NEQ 0 exit /b 0 

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>call powershell.exe -NoLogo -EncodedCommand ZwBpAHQAIABtAGUAcgBnAGUAIABGAEUAVABDAEgAXwBIAEUAQQBEAA== 
Already up to date.

C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build>if 0 NEQ 0 exit /b 0

This PR fixes this issue, and makes merge_script log more readable.

@DrahtBot DrahtBot added the Tests label Sep 7, 2021
@hebasto
Copy link
Member Author

hebasto commented Sep 8, 2021

cc @MarcoFalke

@maflcko
Copy link
Member

maflcko commented Sep 9, 2021

Concept ACK f78cc90

I am not too familiar with ps so I can't review, but feel free to merge this if it seems right

@hebasto
Copy link
Member Author

hebasto commented Sep 9, 2021

I've updated PR description to elaborate the problem.

@MarcoFalke

I am not too familiar with ps so I can't review, but feel free to merge this if it seems right

You could test the fixed behavior of the merge_script by running this branch on your personal Cirrus account. In that case a merging with the base branch must be skipped.

@maflcko
Copy link
Member

maflcko commented Sep 9, 2021

If you tested it in your fork, that should be sufficient. Feel free to merge.

@fanquake fanquake merged commit 17e27dd into bitcoin:master Sep 9, 2021
@hebasto hebasto deleted the 210907-ps branch September 9, 2021 08:12
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hebasto @maflcko @fanquake @DrahtBot