Skip to content
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

Replace Ubuntu 16.04 with CentOS 7 for prebuilds #674

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 2, 2019

  • Remove prebuild settings from standard "linux" job
  • Add centos7 job with prebuild settings
  • Add centos7-devtoolset7 docker image prebuild script

Ref: prebuild/docker-images#8
Ref: #672

For your consideration, no pressure on this if it's reaching too far, I won't be upset if you don't like it!

* Remove prebuild settings from standard "linux" job
* Add centos7 job with prebuild settings
* Add centos7-devtoolset7 docker image prebuild script

Ref: prebuild/docker-images#8
Ref: #672
@vweevers
Copy link
Member

vweevers commented Oct 2, 2019

Thanks so much for this! For me the main concerns are:

  • Increased build time
  • Maintenance going forward. I'm not very familiar with CentOS.

Once prebuild/docker-images#8 is available in Docker Hub, we'll do a test release here and evaluate.

Comment on lines 27 to 29
- os: osx
node_js: node
env: [TEST=1, TEST_ELECTRON=1, BUILD_CMD=prebuild, BUILD_GROUP=darwin-x64]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vweevers Side note. Why isn't osx using if: tag is present?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that job runs tests too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of this is that if: tag is present says "only invoke this if the current commit is tagged, so is probably a release that needs a prebuild". Otherwise it's just a normal test run. So osx gets test runs and prebuilds, but the prebuild-only jobs (arm and alpine) only get invoked when you need a prebuild and they don't get tests.

Maybe it's a good idea to also invoke alpine and centos7 builds on normal commits, not just tagged ones, and run the tests as well inside those containers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running tests in docker containers would make build times too long IMO.

Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose we do a prerelease (and test the result on a bunch of platforms).

@ralphtheninja
Copy link
Member

I propose we do a prerelease (and test the result on a bunch of platforms).

Sounds good!

@vweevers vweevers merged commit afc68b7 into master Oct 4, 2019
@vweevers vweevers deleted the rvagg/centos7-prebuild branch October 4, 2019 06:35
@vweevers
Copy link
Member

vweevers commented Oct 4, 2019

Build time is excellent:

image

@vweevers vweevers mentioned this pull request Oct 4, 2019
vweevers added a commit to Level/rocksdb that referenced this pull request Nov 9, 2019
Using the new prebuildify-cross (prebuild/prebuildify-cross#7).

This makes the prebuilt binary for linux compatible with Debian 8,
Ubuntu 14.04, RHEL 7, CentOS 7 and other flavors with an old glibc.

Following Level/leveldown#674.
vweevers added a commit to Level/rocksdb that referenced this pull request Nov 16, 2019
This makes the prebuilt binary for linux compatible with Debian 8,
Ubuntu 14.04, RHEL 7, CentOS 7 and other flavors with an old glibc.

Following Level/leveldown#674. This rocksdb PR additionally uses
the new prebuildify-cross (prebuild/prebuildify-cross#7). That's a
github dependency for now; waiting for npm ownership.
vweevers added a commit to Level/rocksdb that referenced this pull request Nov 17, 2019
This makes the prebuilt binary for linux compatible with Debian 8,
Ubuntu 14.04, RHEL 7, CentOS 7 and other flavors with an old glibc.

Following Level/leveldown#674. This rocksdb PR additionally uses
the new prebuildify-cross (prebuild/prebuildify-cross#7). That's a
github dependency for now; waiting for npm ownership.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants