Skip to content

ci: Add environment files for all settings #16623

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
merged 1 commit into from
Aug 19, 2019
Merged

ci: Add environment files for all settings #16623

merged 1 commit into from
Aug 19, 2019

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 15, 2019

This moves all environment settings from travis to files in the ci folder. Now, it is possible to easily run each travis configuration with a single command.

@maflcko maflcko force-pushed the 1908-ciEnv branch 3 times, most recently from c52d9c0 to e5ada9a Compare August 15, 2019 20:56
promag
promag reviewed Aug 15, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This approach pollutes the calling bash environment. Should work without export?

Concept ACK.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 16, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16392 (WIP build: macOS toolchain update by fanquake)
  • #16161 (util: Fix compilation errors in support/lockedpool.cpp by jkczyz)
  • #15584 (build: disable BIP70 support by default by fanquake)
  • #12134 (Build previous releases and run functional tests by Sjors)

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.

fanquake
fanquake reviewed Aug 16, 2019
View reviewed changes
@maflcko maflcko force-pushed the 1908-ciEnv branch 4 times, most recently from f181477 to 29f4819 Compare August 16, 2019 02:07
fanquake
fanquake reviewed Aug 16, 2019
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

Concept ACK

I'll run through each of the different ENV builds locally. Have done amd64_asan and amd64_qt5 so far.

For a future change / discussion, it'd be handy if the Docker containers were named. amd64_qt is more useful than peaceful_rubin once there are a bunch floating around.

Also, maybe we could stop (although not destroy) containers automatically after successful completion of the tests?

@maflcko
Copy link
Member Author

maflcko commented Aug 16, 2019

This approach pollutes the calling bash environment.

Can you explain what command exactly does that? I can't find any.

It will potentially pollute your ccache dir and the git repo with build artifacts, but that has nothing to do with bash.

@maflcko
Copy link
Member Author

maflcko commented Aug 16, 2019

Good suggestions @fanquake. I'll probably leave those as follow-up features that can be added.

@Sjors
Copy link
Member

Sjors commented Aug 17, 2019 •
edited
Loading

ACK. I would find it easier if the new files were numbered the same as its equivalent Travis machine.

Recipe to verify the move:

  1. delete export from ci/test/00_setup_env_*
  2. git commit -a --amend
  3. git show --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change

I tried (cloned in /home/travis/bitcoin):

cd bitcoin
FILE_ENV="./ci/test/00_setup_env_amd64_qt5.sh" ./ci/test_run_all.sh

This fails at the configure step:

travis_fold:start:configure
configure: loading site script /home/travis/bitcoin/depends/x86_64-unknown-linux-gnu/share/config.site
configure: loading cache config.cache
configure: error: `CXXFLAGS' was not set in the previous run
configure: error: `CFLAGS' was not set in the previous run
configure: error: in `/home/travis/bitcoin/build':
configure: error: changes in the environment can compromise the build
configure: error: run `make distclean' and/or `rm config.cache' and start over
...
configure:2987: loading cache config.cache
configure:3014: error: `CXXFLAGS' was not set in the previous run
configure:3014: error: `CFLAGS' was not set in the previous run
configure:3051: error: in `/home/travis/bitcoin/build':
configure:3053: error: changes in the environment can compromise the build
configure:3055: error: run `make distclean' and/or `rm config.cache' and start over
...
configure: exit 1

It may have gotten poluted from a previous run, so I tried purging docker instances and nuking the git repository:

docker stop $(docker ps -a -q)
docker container prune
git clean -dfx

Now it runs. Because Docker has root privileges, things very quickly turn into permission hell if you try to run more than one instance.

As a followup, it would be nice if you can run all configurations with a single command (stopping if any fails).

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Aug 19, 2019
Merge bitcoin#16623: ci: Add environment files for all settings
8b42db1 
fa21737 ci: Add environment files for all settings (MarcoFalke)

Pull request description:

  This moves all environment settings from travis to files in the ci folder. Now, it is possible to easily run each travis configuration with a single command.

Top commit has no ACKs.

Tree-SHA512: 989c6b62eb3839eb1fa5461e986496e9660167e2438a789c7588a6fee4f9c37b332782c010fe5c7de8f606bcf98dffb2481d2777cbce88f87cc9f0c42fb2d7fc
@maflcko
Copy link
Member Author

maflcko commented Aug 19, 2019

Now it runs. Because Docker has root privileges, things very quickly turn into permission hell if you try to run more than one instance.

You can't run more than one ci instance on the repo at the same time.

As a followup, it would be nice if you can run all configurations with a single command (stopping if any fails).

I'll leave this as follow-up ;)

@maflcko maflcko merged commit fa21737 into bitcoin:master Aug 19, 2019
@maflcko maflcko deleted the 1908-ciEnv branch August 19, 2019 13:43
@Sjors
Copy link
Member

Sjors commented Aug 19, 2019

You can't run more than one ci instance on the repo at the same time.

I was running them in sequence.

@maflcko
Copy link
Member Author

maflcko commented Aug 19, 2019

That should work as long as you run sudo git clean -dffx (at your own risk, obviously)

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 19, 2019
@sidhujag
Merge bitcoin#16623: ci: Add environment files for all settings
c8c01c9 
fa21737 ci: Add environment files for all settings (MarcoFalke)

Pull request description:

  This moves all environment settings from travis to files in the ci folder. Now, it is possible to easily run each travis configuration with a single command.

Top commit has no ACKs.

Tree-SHA512: 989c6b62eb3839eb1fa5461e986496e9660167e2438a789c7588a6fee4f9c37b332782c010fe5c7de8f606bcf98dffb2481d2777cbce88f87cc9f0c42fb2d7fc
@fanquake fanquake mentioned this pull request Aug 20, 2019
Closed
@theStack theStack mentioned this pull request Feb 6, 2020
Merged
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
@kwvg
merge bitcoin#16623: Add environment files for all settings
18646ee
kwvg added a commit to kwvg/dash that referenced this pull request Dec 13, 2021
@kwvg
merge bitcoin#16623: Add environment files for all settings
5335524
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Docs Tests
Projects
None yet
Milestone
No milestone
Development

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