-
-
Notifications
You must be signed in to change notification settings - Fork 859
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
CI with Github Actions and conda #1690
Conversation
Tests in Chrome generally work, in Firefox there still some issue with selenium setup that needs fixing. |
The latest issue here, is that with conda (without Docker), running, make src/js/pyproxy.gen.js produces a file with C code, instead of Javascript, that Typescript then unsurprisingly fails to parse. I can reproduce this locally as well. So, src/js/pyproxy.gen.js : src/core/pyproxy.* src/core/*.h
# We can't input pyproxy.js directly because CC will be unhappy about the file
# extension. Instead cat it and have CC read from stdin.
# -E : Only apply prepreocessor
# -C : Leave comments alone (this allows them to be preserved in typescript
# definition files, rollup will strip them out)
# -P : Don't put in macro debug info
# -imacros pyproxy.c : include all of the macros definitions from pyproxy.c
rm -f $@
echo "// This file is generated by applying the C preprocessor to core/pyproxy.js" >> $@
echo "// It uses the macros defined in core/pyproxy.c" >> $@
echo "// Do not edit it directly!" >> $@
cat $< | $(CC) -E -C -P -imacros src/core/pyproxy.c $(MAIN_MODULE_CFLAGS) -Isrc/core/ - >> $@ is not working as expected there. |
Seems like this will be resolved at 2021 Q4: github/roadmap#271 |
This should be ready for a review. So far this adds a Linux setup in CI to build core + numpy outside of Docker with conda and corresponding tests in Firefox. The setup is very similar to the one we have in CI. The only change outside of CI is Even though it's a bit redundant the goal is to make sure,
The motivation for conda is to be able to install host python of the required version, other non python dependencies with reasonably recent versions (e.g. @ryanking13 Would you be interested in reviewing? |
Yes I am! |
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 your effort! My comments are:
.github/workflows/main.yml
Outdated
with: | ||
name: core-build-all | ||
path: ~/build-core-all.tgz | ||
retention-days: 5 |
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.
How about matching retention-days of core-build
and core-build-all
?
And also, how about keeping them for a longer period? It looks like we can keep them in maximum 90 days with no disadvantage.
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.
Actually I just realized that we don't really need core-build-all
: it's a left over from a previous config where there were 2 build steps. For debugging we can add other smaller artifacts (e.g. package logs) in the future but for now I'll just remove core-build-all
since it's quite slow.
Also changed core-build retention duration to 60 day following your suggestion. We could put to 90, but there is likely also no need, and it does use storage somewhere, even if we don't pay for it. As far as I remember it's 30 days in CircleCI, and we rarely have an issue with it.
.github/workflows/main.yml
Outdated
run: | | ||
# We create an archive of all packages, as otherwise upload | ||
# is very slow | ||
rm -rf ./src/js/node_modules/ |
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.
I think we can remove many files in emsdk
to reduce archive size, which are not actually build outputs.
For example:
emsdk/emsdk/upstream/emscripten/node_modules
emsdk/emsdk/node
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.
Indeed. For now I removed all of core-build-all
.
.github/workflows/main.yml
Outdated
# This is necessary to use the ccache from emsdk | ||
source pyodide_env.sh | ||
ccache -z | ||
# Build core + numpy. Joblib is required in one of our numpy tests. |
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.
It looks like if we build numpy but not build joblib, the test fails at test_joblib.py
I don't think invoking tests in test_joblib.py are what we expect in this case. Maybe some parameter is misused in pytest?
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.
Indeed. Well the test name test_joblib_numpy_pickle
in test_numpy.py
is OK, but my pytest selection query was not specific enough. Fixed it.
Thanks a lot for the review @ryanking13! |
Ref #1688 (comment)