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.

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

ci: Start with clean env #27976

Merged
merged 3 commits into from
Aug 24, 2023
Merged

ci: Start with clean env #27976

merged 3 commits into from
Aug 24, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jun 26, 2023

Starting with a clean env should help to avoid non-determinism, such as the one fixed in #27739 (comment)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 26, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge
Concept ACK fanquake
Approach ACK jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28116 (test: update tool_wallet coverage for unexpected writes to wallet by jonatack)
  • #27793 (ci: label docker images and prune dangling images selectively by stickies-v)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot changed the title ci: Start with clean env ci: Start with clean env Jun 26, 2023
@DrahtBot DrahtBot added the Tests label Jun 26, 2023
@DrahtBot DrahtBot mentioned this pull request Jun 27, 2023
Merged
@fanquake
Copy link
Member

Concept ACK

@maflcko maflcko force-pushed the 2306-ci-env- branch 2 times, most recently from fa76512 to fa19161 Compare July 11, 2023 20:35
@DrahtBot DrahtBot mentioned this pull request Jul 12, 2023
Merged
This was referenced Jul 17, 2023
Merged
Closed
@jonatack
Copy link
Member

Approach ACK

@maflcko maflcko marked this pull request as draft July 24, 2023 13:18
@DrahtBot DrahtBot mentioned this pull request Jul 24, 2023
Merged
@maflcko
Copy link
Member Author

maflcko commented Jul 25, 2023

Split out an easy-to-test bugfix: #28138 . So please review that one first.

This was referenced Jul 27, 2023
Merged
Merged
This was referenced Jul 31, 2023
Merged
Merged
@maflcko maflcko force-pushed the 2306-ci-env- branch 2 times, most recently from 8ec89e0 to 3ebfafb Compare August 9, 2023 10:36
MarcoFalke added 3 commits August 23, 2023 15:39
ci: Limit scope of some env vars
fa8c250 
No need to have a larger scope than needed.

Can be reviewed with --color-moved=dimmed-zebra
ci: Start with clean env
fa378be 
This should help to avoid non-determinism.
ci: Remove no longer applicable section
fa15f7e 
This fails with:

"Error: determining starting point for build: no FROM statement found"
@dergoegge
Copy link
Member

Is this pull just a doc change? This doesn't look like it actually changes anything to start with a clean env.

@maflcko maflcko marked this pull request as ready for review August 23, 2023 14:12
@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2023

Is this pull just a doc change? This doesn't look like it actually changes anything to start with a clean env.

Yes, it changes the doc to recommend env -i. On my system:

$ env --help | grep 'empty env'
  -i, --ignore-environment  start with an empty environment

@maflcko
Copy link
Member Author

maflcko commented Aug 23, 2023

Oh, was your question "Why didn't you modify ./ci/test_run_all.sh to clear the env?" If yes, the answer would be, to still allow the user to set any or all env vars that can be modified. For example, MAKEJOBS.

dergoegge
dergoegge approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

utACK fa15f7e

@fanquake fanquake merged commit cd5d2f5 into bitcoin:master Aug 24, 2023
15 checks passed
@maflcko maflcko deleted the 2306-ci-env- branch August 24, 2023 08:35
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
@fanquake
Merge bitcoin#27976: ci: Start with clean env
0ad7e12 
fa15f7e ci: Remove no longer applicable section (MarcoFalke)
fa378be ci: Start with clean env (MarcoFalke)
fa8c250 ci: Limit scope of some env vars (MarcoFalke)

Pull request description:

  Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in bitcoin#27739 (comment)

ACKs for top commit:
  dergoegge:
    utACK fa15f7e

Tree-SHA512: 716b264217557b6524dab92d5a2a8d61ecb982dff475bd0cf5a763070b4c5916cd5995e764eb5d67d9cf2428c29d5fc2f42b32941b54c7c3053123ce448171e5
@bitcoin bitcoin locked and limited conversation to collaborators Aug 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dergoegge dergoegge dergoegge approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @fanquake @jonatack @dergoegge