Skip to content

more robust circleci testing#6022

Merged
MartinNowak merged 4 commits intodlang:masterfrom
MartinNowak:cleanup_circleci
Aug 6, 2016
Merged

more robust circleci testing#6022
MartinNowak merged 4 commits intodlang:masterfrom
MartinNowak:cleanup_circleci

Conversation

@MartinNowak
Copy link
Member

No description provided.

circle.yml Outdated
test:
override:
- case $CIRCLE_NODE_INDEX in 0) MODEL=32 ./travis.sh ;; 1) MODEL=64 ./travis.sh ;; esac:
- case $CIRCLE_NODE_INDEX in 0) MODEL=32 ./circleci.sh ;; 1) MODEL=64 ./circleci.sh ;; esac:
Copy link
Contributor

Choose a reason for hiding this comment

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

@MartinNowak this is a hard-coded hack to support parallelism.
However of course, if only one container is enabled for a repo only the 32-bit will be run.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a horrible YAML abomination b/c apparently they aren't capable to allow hashes as argument (and we don't want to use multiline bash scripts as hash key, even in YAML).

@MartinNowak MartinNowak force-pushed the cleanup_circleci branch 7 times, most recently from 86fc186 to 3809f7c Compare August 6, 2016 18:02
- move all the logic into a separate shell script (CircleCI's YAML is
  really unfriendly for slightly more complicated stuff)
- use installer script to fetch bootstrap compiler (same retry logic as
  on Travis-CI)
- cache installed bootstrap compiler
- more consisten with other dmd make options
@MartinNowak MartinNowak changed the title WIP - more robust circleci testing more robust circleci testing Aug 6, 2016
@MartinNowak
Copy link
Member Author

Mainly did this b/c I wanted to restructure the Travis-CI tests to start less workers, but failed due to all the extra cases for CircleCI.
Better to have that in a separate file and deal w/ only a single provider at a time.

@codecov-io
Copy link

codecov-io commented Aug 6, 2016

Current coverage is 87.42% (diff: 100%)

Merging #6022 into master will increase coverage by 0.39%

@@             master      #6022   diff @@
==========================================
  Files            90         90          
  Lines         55686      55686          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          48459      48681   +222   
+ Misses         7227       7005   -222   
  Partials          0          0          

Powered by Codecov. Last update ec75122...48e3640

case $CIRCLE_NODE_INDEX in
0) MODEL=64 ;;
1) MODEL=32 ;;
esac
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please enable two containers, s.t. 32-bit gets tested too?

Should be here https://circleci.com/gh/dlang/dmd/edit#parallel-builds

Copy link
Contributor

Choose a reason for hiding this comment

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

I enabled a second one.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not testing the backend for coverage, shouldn't be much difference, but well.

Copy link
Member

Choose a reason for hiding this comment

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

I asked @wilzbach to stick with one, because the Terms Of Service only offer 4 instances for open source projects. It was unclear what CircleCI's definition of open source was, so while we await clarification I'd like to stay with one. (If their definition is "public repository", then we're golden.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked @wilzbach to stick with one, because the Terms Of Service only offer 4 instances for open source projects.

Did you miss this comment?

I just found the following in the settings at the org page

Additionally, projects that are public on GitHub will build with 3 extra containers -- our gift to free and open source software.

and then this in their FAQ

What if I am building open-source?

We offer a total of four free linux containers ($2400 annual value) for open-source projects. Simply keeping your project public will enable this for you!

#6006 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't think there is any credible risk associated with that, but if it makes you uncomfortable, feel free to just toggle it back in the admin UI (see @wilzbach's link).

If we can get away with only one configuration, this would be preferred either way, though, since parallel PR builds eat into the same allowance.

Copy link
Member

Choose a reason for hiding this comment

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

@wilzbach yes, I had missed that. Glad this is cleared up. Thanks!

@klickverbot Looks like we're good on that, then. But there's that other issue you mentioned - maybe throttling it by using only 1 instance will be more effective for us than running dry halfway through the month.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WalterBright: It's not so much a question of "running dry" than just the latency of parallel builds on many updated PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

than just the latency of parallel builds on many updated PRs.

@klickverbot Let's just see whether it makes a difference and/or clogs our CircleCi queue. It's just an experiment, so we can always switch back to 1 container ;-)

@wilzbach
Copy link
Contributor

wilzbach commented Aug 6, 2016

Mainly did this b/c I wanted to restructure the Travis-CI tests to start less workers, but failed due to all the extra cases for CircleCI.
Better to have that in a separate file and deal w/ only a single provider at a time.

Thanks a lot for this massive cleanup 👍
It looks a lot more professional than my hack.

local base_branch=$(curl -fsSL https://api.github.com/repos/dlang/dmd/pulls/$CIRCLE_PR_NUMBER | jq -r '.base.ref')
else
local base_branch=$CIRCLE_BRANCH
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the snippet to get the target branch. Hopefully we won't hit any rate limit of github, b/c of other people on the test machine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope we don't , otherwise we can try 1) direct authenticated access or 2) rolling our own, small API cache

Copy link
Member Author

Choose a reason for hiding this comment

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

Or extend CircleCI, but I don't it'll be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw didn't you want to merge into the current base_branch?
Something like simple like this should do it?

if [ -n ${CIRCLE_PR_NUMBER:-} ]; then
    git remote add upstream git@github.com:dlang/dmd.git
    git fetch upstream
    git merge "upstream/${base_branch}"
else
    git merge ${base_branch}
fi

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes merging would be good.

@MartinNowak
Copy link
Member Author

Auto-merge toggled on

@MartinNowak MartinNowak merged commit cf0a174 into dlang:master Aug 6, 2016
@MartinNowak MartinNowak deleted the cleanup_circleci branch August 6, 2016 21:54
@JackStouffer
Copy link
Contributor

@MartinNowak This broke building dmd on OS X

$ make -f posix.mak AUTO_BOOTSTRAP=1
/Library/Developer/CommandLineTools/usr/bin/make -C src -f posix.mak
no cpu specified, assuming X86
mkdir -p /tmp/.host_dmd-2.068.2
curl -fsSL http://downloads.dlang.org/releases/2.x/2.068.2 /dmd.2.068.2 .osx.tar.xz | tar -C /tmp/.host_dmd-2.068.2  -Jxf - || rm -rf /tmp/.host_dmd-2.068.2
curl: (3) <url> malformed
curl: (6) Could not resolve host: .osx.tar.xz
tar: Unrecognized archive format
tar: Error exit delayed from previous errors.
CC=c++ /tmp/.host_dmd-2.068.2 /dmd2/osx/bin/dmd -conf=/tmp/ /dmd2/osx/bin/dmd.conf idgen.d
/bin/sh: /tmp/.host_dmd-2.068.2: No such file or directory
make[1]: *** [idgen] Error 127
make: *** [all] Error 2

else
# Auto-bootstrapping, will download dmd automatically
HOST_DMD_VER=2.068.2
HOST_DMD_VER=2.068.2 # keep in sync with other occurences of that variable, e.g. in circleci.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

@MartinNowak This line is the culprit

Copy link
Contributor

Choose a reason for hiding this comment

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

That's bash craziness 😢
@JackStouffer do quotes work for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting the space after the 2 and moving the comment to its own line works

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting the space after the 2 and moving the comment to its own line works

How about quickly submitting this as PR? ;-)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants