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

centos7 image #119

Merged
merged 4 commits into from
Oct 22, 2019
Merged

centos7 image #119

merged 4 commits into from
Oct 22, 2019

Conversation

scopatz
Copy link
Member

@scopatz scopatz commented Oct 21, 2019

New centos7 image for creating centos7 CDTs

@scopatz
Copy link
Member Author

scopatz commented Oct 21, 2019

CC @conda-forge/core

@scopatz
Copy link
Member Author

scopatz commented Oct 21, 2019

This is ready for review!

@scopatz scopatz mentioned this pull request Oct 21, 2019
6 tasks
.travis.yml Outdated Show resolved Hide resolved

# Set an encoding to make things work smoothly.
ENV LANG en_US.UTF-8

Copy link
Member

@mbargull mbargull Oct 21, 2019

Choose a reason for hiding this comment

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

Do we need

Suggested change
ENV LANGUAGE=en_US.UTF-8

as in

ENV LANGUAGE=en_US.UTF-8
for something? If there is no counterargument, I'd suggest to unify this, i.e., either add LANGUAGE or remove it from the other images as well.
https://www.gnu.org/software/gettext/manual/html_node/The-LANGUAGE-variable.html#The-LANGUAGE-variable

Copy link
Member Author

Choose a reason for hiding this comment

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

I think unifying these would be best as another PR targeted at this issue

Copy link
Member

Choose a reason for hiding this comment

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

Then I'd just add it here and decide in another PR if it can be removed.
(Though not strong opinion, if you disagree.)

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM by a quick look (meaning I would appreciate if some else approved as well.)

@scopatz
Copy link
Member Author

scopatz commented Oct 21, 2019

Thanks @mbargull - I also think think this is ready for another review

@jakirkham
Copy link
Member

Would it be possible to build this as a multi-arch image? If so, that would allow us to consolidate the ppc64le and aarch64 images in one image and thus close out issue ( #102 ).

cc @jayfurmanek

@mbargull
Copy link
Member

mbargull commented Oct 21, 2019

Would it be possible to build this as a multi-arch image?

Possible yes, I assume. But that wouldn't be a prerequisite for this PR, I don't think. I've never built multiarch images but AFAIK it's "just metadata". Meaning you can lump different images for different architectures under the same name, but they are different images nonetheless.
(summarized: I agree, but would postpone that for a future PR.)

@scopatz
Copy link
Member Author

scopatz commented Oct 21, 2019

I agree, but postpone that for a future PR.

I would prefer this as well. The goal of this PR is not to rewrite our docker-image infrastructure. It is only to get a centos7 image up that we can use to build CDT packages against.

.travis.yml Outdated
@@ -12,6 +12,11 @@ matrix:
- DOCKERIMAGE=linux-anvil-comp7
- DOCKERTAG=jnlp-slave

- os: linux
env:
- DOCKERIMAGE=linux-anvil-cos7
Copy link
Member

Choose a reason for hiding this comment

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

Are we happy with this naming?

Copy link
Member

Choose a reason for hiding this comment

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

Fine/preferred by me (and suggested by @isuruf).

Do you want to suggest something else? In reference to the potential/future multi-arch image: Would you want a x86_64 suffix here (i.e., linux-anvil-cos7-x86_64) so linux-anvil-cos7 could unambiguously be the name of a multi-arch image?

Copy link
Member

Choose a reason for hiding this comment

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

I am. We can also do linux-anvil:cos7

Copy link
Member

Choose a reason for hiding this comment

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

I like @mbargull's idea as well. How about linux-anvil-x86_64:cos7?

Copy link
Member

Choose a reason for hiding this comment

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

Using linux-anvil-cos7-x86_64 sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

That will cause issues with sorting I think.

I'm not able to follow. What "sorting", what "issues" with it?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry it's kind of hard to explain. conda-smithy sorts variants. Unfortunately it does this with docker_images too. Maybe it shouldn't do that? However given the current conda-smithy behavior we are reliant on the sort order of docker_images being a specific way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Well, smithy's inner workings aren't my strong suit -- I'll happily let you experts decide then ;).

Copy link
Member

Choose a reason for hiding this comment

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

@jakirkham, can you open an issue in conda-smithy about the sorting? We should not sort them and use the order from the original config. (conda-build messes up the sorting, so we can sort them using the original config)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree. Tried my best to describe the problem in issue ( conda-forge/conda-smithy#1183 ). Please fill in and/or correct it as needed, @isuruf 🙂

@jakirkham
Copy link
Member

cc @mingwandroid

Copy link
Member

@mbargull mbargull left a comment

Choose a reason for hiding this comment

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

LGTM as of 9ae23d8 .

@scopatz
Copy link
Member Author

scopatz commented Oct 22, 2019

Shall we merge this then? 😉

@scopatz
Copy link
Member Author

scopatz commented Oct 22, 2019

Ok this is ready for review again!

@isuruf isuruf merged commit 8498e4a into conda-forge:master Oct 22, 2019
@scopatz scopatz deleted the centos7 branch October 22, 2019 18:07
@scopatz
Copy link
Member Author

scopatz commented Oct 22, 2019

Thanks!

@jakirkham
Copy link
Member

Thanks @scopatz! 😄

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.

4 participants