-
Notifications
You must be signed in to change notification settings - Fork 171
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
update test and build workflows to ubuntu 20.04 #2846
Conversation
@@ -81,16 +83,16 @@ jobs: | |||
|
|||
- name: Install dependencies |
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.
Would it make sense to merge it with Setup ubuntu container
step?
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.
the 'setup ubuntu container' was (and probably is) a temporary step i had to add since github had removed support for ubuntu-18 runners. i do not think we need that step, but just keeping it for future when ubuntu 20 support might go away and we might need to readd the
container:
image: ubuntu:18.04
directive for ubuntu-20 :|
Codecov Report
@@ Coverage Diff @@
## master #2846 +/- ##
=============================================
Coverage 56.70668% 56.70668%
=============================================
Files 88 88
Lines 19160 19160
=============================================
Hits 10865 10865
Misses 7699 7699
Partials 596 596
Continue to review full report in Codecov by Sentry.
|
Massively in favour of this! Looks like the build is failing at the moment though |
run: | | ||
go install github.com/golang/protobuf/[email protected] | ||
sudo apt update | ||
sudo apt install -yqq protobuf-compiler |
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.
can be merged with setup ubuntu container
?
5de0eb5
to
257e432
Compare
b7b6003
to
8cd4f74
Compare
8cd4f74
to
6568b36
Compare
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.
Added one comment. It's fine with me, but please with merging to resolve all discussions and comments from @pwilczynskiclearcode
@@ -277,21 +284,23 @@ jobs: | |||
key: ${{ runner.os }}-binaries-tf-${{ github.sha }} | |||
|
|||
- name: Install dependencies | |||
env: | |||
LIBTENSORFLOW_VERSION: "2.12.1" |
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 we still use tensorflow? Don't want to get rid of it everywhere?
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.
Yeah, if we can tear it out without breaking builds then let's do it. I started trying in #2836 but haven't had time to finish it
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.
oh! that requires quite a lot of changes in go code... which i'd rather not touch in this small-ish PR. would rather bump 2836 with changes from this PR and keep tensorflow removal over in that alone.
Co-authored-by: 0xb79orch <[email protected]>
What does this pull request do? Explain your changes. (required)
Updates test and build workflows to use ubuntu 20.04
Specific updates (required)
How did you test each of these updates (required)
this PR workflow runs
Does this pull request close any open issues?
old PR this one is based off on: #2841
Checklist:
make
runs successfully./test.sh
pass