-
Notifications
You must be signed in to change notification settings - Fork 36.9k
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: Make ci system read-only on the git work tree #17423
Conversation
4abda24
to
fa7e764
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Concept Ack! |
I know nothing about docker but Concept ACK, making things that can be made read-only read-only tends to be a good idea both for determinism and to avoid build steps generating clutter in unexpected places. |
This should be easy to review. All it does it add some |
@JeremyRubin you brought this up, can you please give a tested ACK? |
It was on my list of thing to improve either way. A code review should be sufficient, it is really straightforward. (And travis is testing it to some extend already) |
Testing now... Looks like you still need to run a make distclean before running? Would there be any reason to not have travis (now that it's RO on host) run make distclean first in case it's a dirty dir? |
tested ACK fa7523d Ran and was able to re-run
Would prefer, as noted, if the distclean could happen in the copied dir. |
fa7523d ci: Extend docs (MarcoFalke) fa493ef ci: Make ci system read-only on the git work tree (MarcoFalke) fab1333 ci: Remove git from required packages on host (MarcoFalke) fa00393 ci: Make all filesystem operations inside docker (MarcoFalke) Pull request description: Running the ci completely in a docker, without leaving any traces on the host system is not possible right now because the ccache and depends dir needs to be propagated back and picked up by the host for caching. Fixes #17372 ACKs for top commit: JeremyRubin: tested ACK fa7523d Tree-SHA512: 4bce1a0f883bcbdb34abf409bdbc80d420c5da2045d2f9c5536ac433f9e5b490f23df084546c8c049f688b487572bbfc4f9c4029e9e672f4d9279739d066ed2e
Thanks for testing. I've merged this and left the suggestion as a follow up, see #17544. |
fa7523d ci: Extend docs (MarcoFalke) fa493ef ci: Make ci system read-only on the git work tree (MarcoFalke) fab1333 ci: Remove git from required packages on host (MarcoFalke) fa00393 ci: Make all filesystem operations inside docker (MarcoFalke) Pull request description: Running the ci completely in a docker, without leaving any traces on the host system is not possible right now because the ccache and depends dir needs to be propagated back and picked up by the host for caching. Fixes bitcoin#17372 ACKs for top commit: JeremyRubin: tested ACK fa7523d Tree-SHA512: 4bce1a0f883bcbdb34abf409bdbc80d420c5da2045d2f9c5536ac433f9e5b490f23df084546c8c049f688b487572bbfc4f9c4029e9e672f4d9279739d066ed2e
fa7523d ci: Extend docs (MarcoFalke) fa493ef ci: Make ci system read-only on the git work tree (MarcoFalke) fab1333 ci: Remove git from required packages on host (MarcoFalke) fa00393 ci: Make all filesystem operations inside docker (MarcoFalke) Pull request description: Running the ci completely in a docker, without leaving any traces on the host system is not possible right now because the ccache and depends dir needs to be propagated back and picked up by the host for caching. Fixes bitcoin#17372 ACKs for top commit: JeremyRubin: tested ACK fa7523d Tree-SHA512: 4bce1a0f883bcbdb34abf409bdbc80d420c5da2045d2f9c5536ac433f9e5b490f23df084546c8c049f688b487572bbfc4f9c4029e9e672f4d9279739d066ed2e
Running the ci completely in a docker, without leaving any traces on the host system is not possible right now because the ccache and depends dir needs to be propagated back and picked up by the host for caching.
Fixes #17372