Fix coverage CI #194

TotallyNotChase · 2020-10-04T16:45:17Z

Make coverage report CI use checkout@v2, followed by the manual build process . (previously was using gwion-action)
Make "Generate report" step deduce branchname correctly for both push and PR event
Make "Send mail" step only execute when the repository owner is fennecdjay

Update with origin

TotallyNotChase · 2020-10-04T17:24:17Z

looks like the fields for email simply doesn't exist on PRs, even if the correct secret variables are set

fennecdjay · 2020-10-04T17:25:06Z

I really like how you setup this whole work, with the issues before and the summary in the PR body 😄

It's a shame it is failing! 😖
What do you think of the id of keeping only the 101 first lines of the workflow? Mail are only for me, so I think I'd rather check myself coverage when there is a code change than annoy everybody with this mail step. (however, If you have a better idea...)

I was looking at your repo and noticed that all OS tests used gwion-action. I thought I had changed those a while back!

However I think it can easily be solved with a bash trick:

cd .github/workflows
for cifile in linux.yml macos.yml windows.yml
do
  awk '1;/steps:/{exit}' $cifile > $cifile.tmp
  cat << EOF >> $cifile.tmp
    - name Checkout
      uses: actions/checkout@v2

    - name: Init submodules
      run: git submodule update --init util ast

    - name: Build
      run: make
      env:
        CC: \${{ matrix.cc }}
        USE_DOUBLE: \${{ matrix.double }}
        USE_DEBUG: 1

    - name: Test
      run: make test
EOF
  mv $cifile.tmp $cifile;
done
cd ../..

The issue remains to create. Would you be interrested on this one?

TotallyNotChase · 2020-10-04T17:32:32Z

What do you think of the id of keeping only the 101 first lines of the workflow? Mail are only for me, so I think I'd rather check myself coverage when there is a code change than annoy everybody with this mail step. (however, If you have a better idea...)

sounds good.

I was looking at your repo and noticed that all OS tests used gwion-action. I thought I had changed those a while back!

yeah that's what I was talkin about in this comment haha. I will try fixing that, sure.

…nto ci-fixes

fennecdjay · 2020-10-04T17:43:00Z

.github/workflows/coverage.yml

+        if [ "${{ github.event_name }}" == "push" ]
+        then
+          ref="${{ github.ref }}"
+        else
+          ref="${{ github.event.head.ref }}"
+        fi
+        branch=$(basename ref)


Pretty useful snippet here.

.github/workflows/coverage.yml

fennecdjay · 2020-10-04T17:45:05Z

.github/workflows/coverage.yml

+    - name: Init submodules
+      run: git submodule update --init util ast


Submodules on their own place is nice.

fennecdjay · 2020-10-04T17:45:55Z

.github/workflows/coverage.yml

-        dir: .
-        ref: ${{ github.sha }}
-        run: 'true'
+    - name: Checkout


That's the thing asked for in the issue, you uncovered so many things beyond that!
Thank you.

fennecdjay · 2020-10-04T17:51:28Z

You made it all green! That's awesome.
Could you just please change ${{ matrix.cc }} to gcc in windows builds? (see my comment above)
That and confirming you're ready to merge 😄

see my comment below ... I'm not yet famliar with github reviews.

.github/workflows/windows.yml

.github/workflows/linux.yml

.github/workflows/macos.yml

.github/workflows/windows.yml

fennecdjay · 2020-10-04T18:05:57Z

I see the problem: the error in the newly modified yml files was not caught because they only trigger on pushes.
Could be yet another another issue. We need it to trigger on PRs, as we could introduce an error in the main repo/branch and only notice after merging.
The pull-request should trigger on all branches but gh-pages as for the push event.

I can't say how grateful I already am. You did a fair amount of work, pretty seriously as far as I'm concerned. I learned so much, thank you!

TotallyNotChase · 2020-10-04T18:17:26Z

whoops, didn't notice the missing colons

fennecdjay · 2020-10-04T18:18:56Z

Neither did I when I posted the script 😄
It should make things greener on your side.

TotallyNotChase · 2020-10-04T18:21:17Z

Ah, windows with its weird design choices again. Looks like %z is not being recognized as a valid format specifier

fennecdjay · 2020-10-04T18:22:37Z

Ah, windows with its weird design choices again.

Can relate 😄

TotallyNotChase · 2020-10-04T18:24:52Z

This SO thread may be helpful. Should this be a fix on the code side then?

fennecdjay · 2020-10-04T18:25:57Z

Has your windows build BUILD_ON_WINDOWS set? this would address the %z issue.

The problem might be cannot find -lasan.
I think the solution to this is in util/common.k, where before
ifeq (${USE_DEBUG}, 1), there should be something along the lines of

ifeq (${BUILD_ON_WINDOWS}, 1)
USE_DEBUG=0
endif

so that windows builds do not try to link with sanitizer libraries, since their are not implemented as of now.

TotallyNotChase · 2020-10-04T18:29:28Z

Would setting USE_DEBUG to 0 here work

Edit: and also replacing that with BUILD_ON_WINDOWS: 1

fennecdjay · 2020-10-04T18:33:51Z

I think that's the trick 😄

fennecdjay · 2020-10-04T18:36:58Z

TBH, I can't wait to see it all green (because I think it will 😄)

fennecdjay · 2020-10-04T18:42:49Z

I was watching at the window testing live, so to say, and I think we need BUILD_ON_WINDOWS set in the testing step too, because when it run tests/sh/import.sh, some plugins are compiled, and that needs the variable set to run smoothly.

Also, the test suite fails to catch the error 😸

TotallyNotChase · 2020-10-04T18:46:46Z

Could you tell me what modifications this needs? Should the build and test be separated?

fennecdjay · 2020-10-04T19:06:22Z

TBH, I not sure what your this link refers to.

Assuming it's refering to

Also, the test suite fails to catch the error

First, we need to add set -e to tests/sh/import.sh.

It looks like the test_plugin function in tests/sh/import.sh does not run the test (while it set the options do run it correctly (incorrect, see footnote)).

It needs to call run, defined in tests/sh/common.sh.
But this function calls ./gwion, as if we where in the root directory of the project.

So the tests should be run for the root.
We have to remove the cd test/import line and make test_plugin work from the root directory. Hopefully I think it can be solved by adding the -C flag (as in changing make to make -C <import_test_directory>. As for the make clean line.
Remains to find how to handle the <import_test_directory> thing.
at the end of tests/sh/import.sh there will also be no need for cd "$BASE_DIR" || exit

There is GWOPT+=-p. test_gw in tests/sh/import.gw.
It should be changed to GWOPT+=-p <import_test_directory>.
The test_gw is totally irrelevant.

TotallyNotChase · 2020-10-04T19:12:40Z

Ahh I see. I was referring to the conversation on Build and Test gwion in coverage.yml actually. That this link seems to just scroll up to it and puts the comment at the very top of the screen.

As for the tests/sh changes, I think it'd be better to work on that on a separate PR, since those aren't directly related to the action files, right?

fennecdjay · 2020-10-04T19:12:42Z

Fix thing mentionned in my previous comment is totally up to you.
The changes you did in this PR are totally worth merging. Just let me know.

fennecdjay · 2020-10-04T19:13:39Z

I think it'd be better to work on that on a separate PR

That's also better IMO.

fennecdjay · 2020-10-04T19:15:51Z

I was referring to the conversation on Build and Test

It's easier when maintaining to have separate steps to look for for build and and test, so I think these should be separated.

TotallyNotChase · 2020-10-04T19:19:56Z

Extended changelog:-

Remove "Send mail" step from coverage.yml
Use manual checkout + make build in linux.yml, macos.yml, and windows.yml - replaces gwion-action

fennecdjay · 2020-10-05T09:18:22Z

Just reviewed your changes. Looks good to me!
Did I forgot something or are you OK to merge?

TotallyNotChase · 2020-10-05T09:52:17Z

I think that should be all for the CI fixes. Should be good to merge

fennecdjay · 2020-10-05T10:16:14Z

I think so.
Merging right now.
Thanks for the great work!

TotallyNotChase added 4 commits October 4, 2020 11:22

Merge pull request #1 from fennecdjay/master

d01788f

Update with origin

🔧 Use checkout + make instead of gwion-action for coverage

e5128b7

🔧 Fix branchname resolution during report generation

1407fcb

🔧 Make "Send mail" only execute in original repo

374628f

TotallyNotChase added 3 commits October 5, 2020 01:38

🔧 Remove "Send mail" step from coverage report

a3e0ecc

🔧 Remove gwion-action from linux/macos/windows workflows

d8e33cd

Merge branch 'ci-fixes' of https://github.com/TotallyNotChase/Gwion i…

ca71c60

…nto ci-fixes

fennecdjay reviewed Oct 4, 2020

View reviewed changes

.github/workflows/windows.yml Outdated Show resolved Hide resolved

fennecdjay reviewed Oct 4, 2020

View reviewed changes

.github/workflows/linux.yml Outdated Show resolved Hide resolved

.github/workflows/macos.yml Outdated Show resolved Hide resolved

.github/workflows/windows.yml Outdated Show resolved Hide resolved

TotallyNotChase added 4 commits October 5, 2020 02:11

🔧 Remove comments from the end of file

90173f4

🔧 Fix missing colon after name

0530873

🔧 Set cc back to gcc for windows

5bcd603

🔧 Make linux/macos/windows.yml actions run on PRs as well

6f9c888

🔧 Set BUILD_ON_WINDOWS in windows.yml

425ece7

🔧 Set BUILD_ON_WINDOWS on the test step as well

c84b193

🔧 Separate steps for building and testing in coverage

7803344

fennecdjay merged commit 99eab3c into Gwion:master Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix coverage CI #194

Fix coverage CI #194

TotallyNotChase commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay Oct 4, 2020

fennecdjay Oct 4, 2020

fennecdjay Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 5, 2020

TotallyNotChase commented Oct 5, 2020

fennecdjay commented Oct 5, 2020

		- name: Init submodules
		run: git submodule update --init util ast

Fix coverage CI #194

Fix coverage CI #194

Conversation

TotallyNotChase commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay Oct 4, 2020

Choose a reason for hiding this comment

fennecdjay Oct 4, 2020

Choose a reason for hiding this comment

fennecdjay Oct 4, 2020 • edited

Choose a reason for hiding this comment

fennecdjay commented Oct 4, 2020 • edited

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 • edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 • edited

TotallyNotChase commented Oct 4, 2020 • edited

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 • edited

fennecdjay commented Oct 4, 2020 • edited

TotallyNotChase commented Oct 4, 2020 • edited

fennecdjay commented Oct 4, 2020 • edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020

fennecdjay commented Oct 4, 2020 • edited

TotallyNotChase commented Oct 4, 2020

fennecdjay commented Oct 5, 2020

TotallyNotChase commented Oct 5, 2020

fennecdjay commented Oct 5, 2020

fennecdjay Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

TotallyNotChase commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited

fennecdjay commented Oct 4, 2020 •

edited