-
Notifications
You must be signed in to change notification settings - Fork 2
replace conda/nuitka with standalone python builds [CPP-310][CPP-322][CPP-321] #146
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
Conversation
873ec18
to
5c9c5e9
Compare
bbc6932
to
bd349d8
Compare
bd349d8
to
c6ad4f4
Compare
3d850ec
to
c56c425
Compare
adc2aa0
to
96d6c0b
Compare
I think we will need to reevaluate the caching in CI after this PR gets merged it looks like mac has a fairly brutal benchmark time ~40m (12m build and 20+ benchmark). I may be able to look into this next sprint. |
Hmmm the other runs did not take that long but I don't see what changed |
Ah it must be stuck then. I dont think there is a set timeout on the benchmarks so we may need to just kill it and hope for a useful backtrace. |
set_env PYTHON "${WORKSPACE}/py39/bin/python3" | ||
set_env DIST_PYTHON "${WORKSPACE}/py39-dist/bin/python3" | ||
set_env PYSIDE2_RCC "${WORKSPACE}/py39/bin/pyside2-rcc" | ||
set_env BACKEND_WHEEL console_backend-0.1.0-cp39-cp39-macosx_10_15_x86_64.whl |
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.
In a future PR, I think we will need to come up with a more clever way to populate these environment variables to work with other OS versions. I get an error running cargo make create-dist
on my mac:
creating 'dist/console_backend-0.1.0-cp39-cp39-macosx_11_0_x86_64.whl' and adding 'build/bdist.macosx-10.9-x86_64/wheel' to it
adding 'console_backend/__init__.py'
adding 'console_backend/server.cpython-39-darwin.so'
adding 'console_backend-0.1.0.dist-info/METADATA'
adding 'console_backend-0.1.0.dist-info/WHEEL'
adding 'console_backend-0.1.0.dist-info/top_level.txt'
adding 'console_backend-0.1.0.dist-info/RECORD'
removing build/bdist.macosx-10.9-x86_64/wheel
[cargo-make] INFO - Execute Command: "/Users/jm/dev/console_pp/py39-dist/bin/python3" "-m" "pip" "install" "console_backend/dist/console_backend-0.1.0-cp39-cp39-macosx_10_15_x86_64.whl"
WARNING: Requirement 'console_backend/dist/console_backend-0.1.0-cp39-cp39-macosx_10_15_x86_64.whl' looks like a filename, but the file does not exist
Processing ./console_backend/dist/console_backend-0.1.0-cp39-cp39-macosx_10_15_x86_64.whl
ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/Users/jm/dev/console_pp/console_backend/dist/console_backend-0.1.0-cp39-cp39-macosx_10_15_x86_64.whl'
[cargo-make] ERROR - Error while executing command, exit code: 1
[cargo-make] WARN - Build Failed.
(base) jms-MacBook-Pro:console_pp jm$
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.
We should be able to force the build to produce *macosx_10_15_x86_64*
consistently by setting MACOSX_DEPLOYMENT_TARGET
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.
Do you mean by just adding MACOSX_DEPLOYMENT_TARGET=macosx_10_15_x86_64
as an env variable?
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 it needs to be formatted as MACOSX_DEPLOYMENT_TARGET=10.15 but yeah, let's try that
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.
@john-michaelburke just pushed a change setting that, could you try building the dist again when you have a chance?
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.
@notoriaga Everything looks good on my end!
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.
Ran:
create-dist: Ubuntu looks good, Windows looks good, Mac had an issue with version 10.5 vs 11.0 see other comment.
run: Ubuntu / Windows / Mac all look good
frontend-mem-bench: Ubuntu and Windows both worked well
LGTM might be the MVP PR of the quarter tied with the Settings tab work 😄
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.
Looks great so far, have we tried tagging a test release with this PR yet?
Just tried one, seems good! https://github.com/swift-nav/console_pp/releases/tag/0.10.0-test-2021-10-13 |
The double date is because I added it to the tag not realizing the gh action added it too |
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.
LGTM @notoriaga please squash things down to one commit or multiple commits, as you see fit, also would be great for the commit message to have a (light) summary of the changes
No description provided.