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

Add Intel UP2 support with CentOS 7 #2093

Merged
merged 1 commit into from May 23, 2018

Conversation

Projects
None yet
3 participants
@ctron
Copy link
Contributor

ctron commented May 9, 2018

Add support for Intel UP² based on CentOS 7

Note: This PR also requires #2091 to be fixed. Possibly by merging #2092

@ctron ctron requested a review from MMaiero May 9, 2018

@ctron ctron force-pushed the ctron:feature/intel_up2_1 branch 3 times, most recently from de97078 to 9d74569 May 9, 2018

@@ -0,0 +1,157 @@
#
# Copyright (c) 2011, 2017 Eurotech and others

This comment has been minimized.

@MMaiero

MMaiero May 10, 2018

Contributor

2018?

@@ -0,0 +1,121 @@
#!/bin/sh
#
# Copyright (c) 2016 Red Hat Inc and others

This comment has been minimized.

@MMaiero

MMaiero May 10, 2018

Contributor

2018?

Same for other files contributed here.

@MMaiero

This comment has been minimized.

Copy link
Contributor

MMaiero commented May 10, 2018

@dwoodard1 I don't have a board to do some testing. Could you do a check with yours?

@ctron ctron force-pushed the ctron:feature/intel_up2_1 branch from 9d74569 to b5ba1ac May 14, 2018

@ctron ctron force-pushed the ctron:feature/intel_up2_1 branch from b5ba1ac to f172b59 May 14, 2018

@ctron

This comment has been minimized.

Copy link
Contributor

ctron commented May 14, 2018

@MMaiero I added 2018 to the copyright headers. But only when they existed and I actually modified the file.

@MMaiero

This comment has been minimized.

Copy link
Contributor

MMaiero commented May 14, 2018

@ctron Do you have any hurry on this? I would like to keep on hold until we get feedback from Dave. Just to verify that everything works as expected with a proper QA validation.

@ctron

This comment has been minimized.

Copy link
Contributor

ctron commented May 14, 2018

I don't see anything which speaks against merging that now. It doesn't change Kura itself, just adds a new distribution profile. And letting the PR wait and longer, only causes more trouble later on when it has to be re-based and possible conflicts being resolved.

And I also don't really understand what you mean by proper QA validation at this point. That is normally done right before the release of Kura. And when Kura enters that phase, then changes like that won't get merged as they are deemed too risky to be merged. At least that was the argument in the past.

So unless anything speaks against merging this, then I would say, merge it now.

@dwoodard1

This comment has been minimized.

Copy link
Contributor

dwoodard1 commented May 21, 2018

Sorry, just now getting around to this. Question, why are we targeting CentOS? If I'm not wrong, the UP2 comes with Ubuntu (at least mine did). Isn't installing CentOS before installing Kura an unnecessary step?

@ctron

This comment has been minimized.

Copy link
Contributor

ctron commented May 22, 2018

Well you may be targeting Ubuntu as well. But my focus is on targeting CentOS. And I think in the spirit of being open source, there is nothing wrong with that approach.

@dwoodard1

This comment has been minimized.

Copy link
Contributor

dwoodard1 commented May 22, 2018

Of course I have no issue supporting CentOS. I think it is a nice addition. I just didn't understand starting here, since unlike the Raspberry Pi, the UP2 ships with an installed OS. I will create a separate PR for Ubuntu support, since I assume that will be the simplest path for new Kura/UP2 users.

Since this doesn't impact the framework directly, only adds a new distribution, I think we can merge. If we find issues we can create separate issues. I can test this after I have tested the default Ubuntu installation. @ctron did you want to make any changes before I merge?

@ctron

This comment has been minimized.

Copy link
Contributor

ctron commented May 23, 2018

Agreed. No please go ahead and merge. As you said, if we find issues we can always improve.

@dwoodard1 dwoodard1 merged commit ff9c735 into eclipse:develop May 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment