Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Python3 migration & Emscripten update #133
Python3 migration & Emscripten update #133
Changes from all commits
625e40c
75af6d7
ca40148
774f1e8
6ea3257
ebc7928
04c2a62
c5f5d89
794d4d1
eb83a1d
b31ff3e
e43fe0a
9fb656d
9bd602f
a9d49b3
6b4b322
03dfd35
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
same
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 can remove these comments if the entrypoint issues are resolved
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.
yes, I didn't find any issues with the latest emscripten base docker image
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.
However I can see the CI build workflow failing because it grabs the pre-built docker image from Docker Hub, so the change to the newer emscripten version is not reflected on the CI workflow.
I wonder if there is a way to build the image from the Dockerfile on the github CI workflow for these changes to the build pipeline instead of grabbing from Docker Hub...
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's definitely possible to build the Docker image in a GitHub workflow, but I should create a new build-dev workflow for when we commit changes to the Dockerfile itself and the other build scripts.
I can do that on this PR to ensure that the other changes result in a successful build process.
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 have done this now and checks are passing. I would remove the files being watched on the new "build-dev" workflow from the other two workflows (integration.yml and deploy.yml), let me know what you think about that @albincorreya
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.
Great job! We could remove these comments in build-libs.sh since it is not needed anymore?
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 assume we will migrate these old python2 scripts to python3 after fixing the build pipeline?
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.
as above, my intention is to do it on this PR
I think all scripts now are compatible with Python 3, but let me know if you see anything that should clearly be changed
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 like
code_generator.py
script has python2 string formatting, not sure if python3 supports that? maybe I missed something hereThere 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.
@albincorreya I see what you mean. It seems that modulo string formatting still works in Python3, although f-strings are the preferred syntax. If it's okay with you, I think it makes sense to do the switch to f-strings on the
code_generator.py
script on the object-oriented PR that I'm working on, because that will already change significant portions of the script.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.
@jmarcosfer make sense, let's merge this then 💯