Move tools checkout to style_lint to avoid failing PRs

[wilzbach]

I observed the following on #5557:

libdparse 0.7.0: building configuration "library"...
../.dub/packages/libdparse-0.7.0/libdparse/src/dparse/ast.d(1346,10): Deprecation: cannot implicitly override base class method object.Object.opEquals with dparse.ast.Declaration.opEquals; add override attribute
/home/ubuntu/phobos/../dmd/generated/linux/release/64/dmd failed with exit code 1.
make: *** [generated/linux/release/64/tests_extractor] Error 2

In setup_repos we merge with the master branch, thus only the later targets have an updated circleci.sh.
A long-term solution might also simply be building DScanner without -de, but this is the simpler fix for now (the other requires a bit of Makefile fiddling).

CC @CyberShadow

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[RazvanN7]

Looking good.

[CyberShadow]

Err, I'm having some difficulty understanding how this change fixes things.

[wilzbach]

Err, I'm having some difficulty understanding how this change fixes things.

    - ./circleci.sh setup-repos
    - ./circleci.sh style_lint
    - ./circleci.sh publictests
    - ./circleci.sh coverage

Every entry in the circle.yml is a separate script invocation. On the first (setup-repos) we merge the PR branch into master. Currently the PRs fails because we clone tools in setup-repos as they clone an older version of tools without the deprecation fixes in libdparse (upgrade PR: dlang/tools#245). Now by moving the clone to the newest circleci.sh the updated tools hash is available and builds can pass without a rebase.

[wilzbach]

An alternative fix would be:

diff --git a/circleci.sh b/circleci.sh
index 443f74b54..8d7ad6daf 100755
--- a/circleci.sh
+++ b/circleci.sh
@@ -129,7 +129,7 @@ coverage()
 # extract publictests and run them independently
 publictests()
 {
-    make -f posix.mak -j$N publictests DUB=$DUB
+    make -f posix.mak -j$N publictests DUB=$DUB DISABLE_DEPRECATION_HALT=1
 }
 
 case $1 in
diff --git a/posix.mak b/posix.mak
index fee516c25..0c2fd74b7 100644
--- a/posix.mak
+++ b/posix.mak
@@ -115,7 +115,7 @@ else
 endif
 
 # Set DFLAGS
-DFLAGS=-conf= -I$(DRUNTIME_PATH)/import $(DMDEXTRAFLAGS) -w -de -dip25 $(MODEL_FLAG) $(PIC)
+DFLAGS=-conf= -I$(DRUNTIME_PATH)/import $(DMDEXTRAFLAGS) -w -dip25 $(MODEL_FLAG) $(PIC)
 ifeq ($(BUILD),debug)
        DFLAGS += -g -debug
 else
@@ -123,6 +123,12 @@ else
 endif
 
 ifdef DISABLE_DEPRECATION_HALT
+DFLAGS  += -d
+else
+DFLAGS  += -de
+endif
+
+ifdef ENABLE_COVERAGE
 DFLAGS  += -cov
 endif

Though it's a good idea to always have the latest tools (as specified in circleci.sh).

[CyberShadow]

Now by moving the clone to the newest circleci.sh the updated tools hash is available and builds can pass without a rebase.

Sorry, I still don't fully understand. How does changing when we do something (without changing what is done) change the situation?

[wilzbach]

How does changing when we do something (without changing what is done) change the situation?

Because we merge with master on the first execution of the script (setup-repos).
On latter executions (style_lint, publictests, coverage) , the script has been updated and thus the git checkout uses the hash from master and not the one from PR.

However, as mentioned we could make building with -de opt-out.
Both solutions would work - in fact they are additive (we could apply both).

[CyberShadow]

Thanks for the explanation. Finally wrapped my head around this.

This seems like a deficiency in Circle CI: https://discuss.circleci.com/t/show-test-results-for-prospective-merge-of-a-github-pr/1662

In this case, it may be better to simply run as little code as possible from the current branch, before merging and handing over the rest of the work to the script on master.

[wilzbach]

Thanks for the explanation. Finally wrapped my head around this.

Sorry for not explaining it properly.

This seems like a deficiency in Circle CI: https://discuss.circleci.com/t/show-test-results-for-prospective-merge-of-a-github-pr/1662

Another thing that we could do on our side would be to automatically restart the last X PRs in the queue when PR has been merged:

dlang/dlang-bot#132

In this case, it may be better to simply run as little code as possible from the current branch, before merging and handing over the rest of the work to the script on master.

Well it's a two-handed sword. If we use only one invocation, CircleCi can't group them anymore as nicely as now:

And people will have a lot more troubles finding errors.