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

ansible: new centos6 machines, updated & fixed centos6 config #1076

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 9, 2018

New set of CentOS 6 machines. Ansible config didn't quite work for it (I think it was written after deployment and hasn't been used to deploy machines from scratch).

Sadly there's something wrong with the SCL devtoolset-2 repo so I've switched to the centos/tru ones which are unsigned but we can get away without errors on that.

rvagg

This comment was marked as off-topic.

@rvagg
Copy link
Member Author

rvagg commented Jan 9, 2018

opened a PR to allow x86_64 as an alias, would like to hear opinions on this change nodejs/node#18052

@gibfahn
Copy link
Member

gibfahn commented Jan 9, 2018

opened a PR to allow x86_64 as an alias, would like to hear opinions on this change nodejs/node#18052

Definitely in favour of just doing in in ./configure rather than duplicating it in every build script.

@mhdawson
Copy link
Member

mhdawson commented Jan 9, 2018

+1 to in ./configure as well.

@rvagg
Copy link
Member Author

rvagg commented Jan 9, 2018

So, we set ARCH / DESTCPU explicitly on release machines, we haven't done it as much on non-release but in this case CentOS 6 is used for releases so it ends up being in our test machines too. My preference is to be explicit about architecture on all our machines rather than letting configure guess; but it's not a strong preference and I know others have preferred leaving it out. I'm fine as long as we be explicit on our release machines.

@mhdawson
Copy link
Member

mhdawson commented Jan 9, 2018

I'm not as worried about having to set it for our test machines, but when people checkout/build it would be good if it just worked whenever possible.

@gibfahn
Copy link
Member

gibfahn commented Jan 10, 2018

My preference is to be explicit about architecture on all our machines rather than letting configure guess; but it's not a strong preference and I know others have preferred leaving it out.

If we're going to be explicit, as much as possible I'd rather we be explicitly saying ARCH=$(uname -m) rather than manually setting it to what we think the ARCH should be. Putting the conversion in ./configure makes it a lot more visible than having it be an option in a template (see #1076 (comment)).

I'm not as worried about having to set it for our test machines, but when people checkout/build it would be good if it just worked whenever possible.

It already worked manually if you didn't specify the ARCH, this is just a question of whether it works if you specifically say ARCH=$(uname -m).

@maclover7
Copy link
Contributor

@rvagg is this needed for #1074 to be resolved? Are there any corresponding changes to the secrets repo which need to be made?

@rvagg
Copy link
Member Author

rvagg commented Feb 14, 2018

No, unrelated to #1074. We're in a bit of a stale-mate of sorts here, suggestions on how to proceed are welcome.

I'd really like to define ARCH and DESTCPU, mainly for the release machines where being explicit is especially important (IMO), and CentOS 6 is now a release machine type.

nodejs/node#18052 got merged but that's only on master for now so this is still a problem for everything but master and will be for some time even if we backport it all the way (unlikely for 4.x I reckon).

@gibfahn
Copy link
Member

gibfahn commented Feb 15, 2018

For now can we just define it in the job (with a comment that says this can be removed when v4.x goes EoL)?

@seishun seishun mentioned this pull request Mar 30, 2018
@rvagg rvagg closed this Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants