-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Create actions for Windows and Mac OS #528
Create actions for Windows and Mac OS #528
Conversation
# Conflicts: # .github/scripts/trailing-newlines.sh # Makefile # qt/tools/typecheck-setup.sh # react/Makefile # rspy/Makefile # svelte/Makefile # tslib/Makefile
before running tests and the build to properly measure the total time of each stage.
its regeneration by hand.
…_for_windows_macos # Conflicts: # .github/scripts/trailing-newlines.sh # .github/workflows/checks.yml # Makefile # pylib/Makefile # qt/Makefile # qt/tools/typecheck-setup.sh # rspy/Makefile
trailing backslash on checks.yml
Do you know why this test is failing? https://github.com/ankitects/anki/runs/532323156?check_suite_focus=true#step:18:477
Perhaps it is related to the order I call the make rules, i.e., first This error is happening in all 3 platforms, including Mac OS. The Mac OS build is not failing because the Mac OS make is not stopping errors: https://github.com/evandroforks/anki/runs/532403941?check_suite_focus=true#step:18:632
I am going to look into why Mac OS make is not stop on errors. |
After I figure out what is wrong, I am going to squash into a single commit all these |
The macOS make is too old to support SHELLFLAGS, which is why the set -eo lines are necessary I haven't had a chance to review this all yet, but it sounds great - thank you for all your hard work on this. |
re the test failing, perhaps it's another issue related to the time of day - it's right around 4am in GMT time |
Ah thanks! Then, I just need to ask I am about to go to sleep now. Tomorrow I should continue digging into it. |
I think it's best if the set -eo commands are returned instead of requiring Mac users to install make as well. Sleep well :-) |
Thanks!
You are right. I will keep the set -e thing. (It is already back on my commits few minutes ago). Now all tests are passing. I am going to try revert back a few things to see if they crash again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your hard work on this! It's looking pretty good already, but the checks.yml could do with some tweaks - I've added some comments below.
870dbb1
to
89de8aa
Compare
Mac OS uses an old version of make which does not support the .SHELLFLAGS feature.
Accordingly to the documentation the name is lower case: https://help.github.com/en/actions/reference/virtual-environments-for-github-hosted-runners
repository with a `repo-token` until know whether it is safe to pass the GitHub token around: https://github.com/actions/virtual-environments/issues 602
…_for_windows_macos
ef232f5
to
8fdc8f0
Compare
8fdc8f0
to
76bf3f8
Compare
These are the results of the lastest tests (no cache https://github.com/ankitects/anki/actions/runs/64172038) vs (with full cache https://github.com/ankitects/anki/actions/runs/64185052)
As of now, I do not think that the caches of By disabling them, it would save us |
c973a43
to
04beb7c
Compare
These are the new results of the lastest tests compared with the first batch (no cache https://github.com/ankitects/anki/actions/runs/64240676) vs (with full cache https://github.com/ankitects/anki/actions/runs/64256407)
Looking at them, the times seem worst than before. After checking, I noticed rust was wasting time downloading all the crates (just for nothing because it was not compiling them). Hopefully Python |
cargo was downloading them just for nothing. ankitects#528
36229f5
to
fe5cb5c
Compare
Haha. Now, it is time for that failing test to come back and haunt us again:
I am going to disable it until this is merged. |
b022462
to
5decee9
Compare
Just taking a screenshot for recordation before I force-push this branch again: These are the results of the lastest tests, after re-enabling the rust cargo index/registry caches (no cache https://github.com/ankitects/anki/actions/runs/64280921) vs (with full cache https://github.com/ankitects/anki/actions/runs/64287568)
Despite the increment in space usage, now the build times are better than the first time. Of course, these times are just an approximation and not all builds should take them exactly. I would expect that a cached build to variate up to 3 minutes more or less, while a build fully rebuilding the cache should take about 5 minutes to more or less. Now force-pushing this branch, reenabling that failing test I had removed. |
8af18c5
to
0c77e0c
Compare
(please let me know when you'd like me to do a final review + merge) |
0c77e0c
to
a8524fb
Compare
cargo was downloading them just for nothing. https://github.com/ankitects/anki/pull 528
a8524fb
to
3f8cc60
Compare
I have finished it. I just forced push to remove duplicated jobs (actions/runner#391). |
Thank you very much for your work on this Evandro! |
I saw you recently created the
uses: ankitects/setup-protoc@master
, becausesetup-protoc
was not working. I also had the same problem when I was creating the GitHub actions for Mac OS. The solution come after opening a issue on (actions/runner-images#602 - API rate limit exceeded for Mac OS hosted by GitHub itself). The problem was that the README.md of thesetup-protoc
project was not updated to use the correct version ofsetup-protoc
. I opened a issue there, asking for their README.md to be updated (arduino/actions#23 - Mark the repository as deprecated). Meanwhile, on this pull request, I am already using the correct version of thesetup-protoc
actions as suggested by actions/runner-images#602.build develop
because while I was building this I got a case wheremake check build
was working/passing, butmake develop
not. I put the develop rule first because it should be the most important, i.e., a user can run the develop build without errors. Then, I check whether Anki can pack itself into python wheels and finally I run all unit tests. Also, by puttingdevelop
beforecheck
allows to each one of them build half of things. Ifcheck
is the first rule, it build all rust libraries at once instead of just part of them.make develop
,make check
andmake build
in different rules just to:pyenv
directory, they are rebuild when you edit aMakefile
or somerequirements.txt
file.pacman
, they are rebuild every time you edit thewindows_checks.yml
file.key:
and increment its the number at the end of the line or just by changing anything else on this composition.SHELLFLAGS
to allow the user to easily debug installation errors by defining the environment variableSHELLFLAGS=-x
before running anything. The flag-x
will cause bash to print out every command it is running. This is vital to figure out how to make the GitHub Actions to work.-u
flag to all shell scripts. AllMakefile
s were already using it, then, I decided to normalize it and keep everybody using it. The-u
flags causes the shell script to fail if it attempts to use a variable which is not defined. If a variable can be undefined, we need to explicitly say it by adding this before using it:rename
script on Windows because thecmd
terminal has a builtin command calledrename
, this it was always taking precedence over the perl scriptrename
. To fix it, I directly call it withperl rename
on windows. I also could not figure out where should I put thisrename
script, then, I just added it to.gitignore
and used a simplecurl
to download it.Cygwin
or themsys2
packages, the only way to make it working was using the Unix Environment already shipped withGit for Windows
. The only problem is that it does not have a package manager. Then, I installed one (pacman, Update/Add Git for windows SDK actions/runner-images#596 - Update/Add Git for windows SDK).Makefile
s asMAKEFLAGS += --warn-undefined-variables
to the top of theMakefile
s because they only start working after they are defined.@set -eo pipefail &&
which the mainMakefile
had because it was redundant. You were already setting the make setting.SHELLFLAGS := -eu -o pipefail
which makes all command to be run through shell with the flags-eu -o pipefail
.make build
command (on Windows), and I figure out it was not working due this problem (Expectedpython
to be a python interpreter inside a virtualenv PyO3/maturin#283 - Expectedpython
to be a python interpreter inside a virtualenv). Then, to fix it I had to manually edit theanki/pyenv/Scripts/activate
script. Also, I am not sure why, but thePython
interpreter on the GitHub Actions Windows was create the fileanki/pyenv/Scripts/activate
withCRLF
line endings, which I had to convert to LF, other the shell script would never be sourced.Cache cargo rslib
andCache cargo rspy
are doing nothing as of now. I just put them there in case these directories are generated some day.make build
: https://github.com/evandroforks/anki/actions/runs/62647001