-
Notifications
You must be signed in to change notification settings - Fork 33
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
Replicated copyright change to open embedded, and resolved multi-license ToDo #100
Conversation
ret += ' ' | ||
first = False | ||
ret += lic | ||
ret += ' '.join(self.license) |
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.
This prints the licences twice, once in a list format and once joined by strings. It's probably worth removing the above print and joining on ', ' for human readability. @allenh1 I assume this is optimized for human readability so comma separated list is best?
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.
@tfoote I'm actually not sure how Open Embedded wants multiple licenses to be handled... I think I'll just pull this change locally and try it out? I'm honestly unsure.
tests/ebuild/simple_expected.ebuild
Outdated
@@ -8,7 +8,7 @@ inherit ros-cmake | |||
|
|||
DESCRIPTION="an ebuild" | |||
HOMEPAGE="https://www.website.com" | |||
SRC_URI="https://www.website.com/download/${PN}/archive/${PN}/release/lunar/0.0.0.tar.gz -> ${PN}-lunar-release-${PV}.tar.gz" | |||
SRC_URI="https://www.website.com/download/stuff.tar.gz -> ${PN}-lunar-release-${PV}.tar.gz" |
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.
This change shouldn't be made -- perhaps this got pulled in during the rebase somehow?
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.
Thats what probably happened, I'll revert that now 👍
… during the rebase
@shaneallcroft Ok, there's an odd way that OE wants this handled.
Shouldn't be that difficult to adapt to, but lmk if you run into any issues. |
@@ -136,16 +138,8 @@ def get_recipe_text(self, distributor, license_text, die_msg=None): | |||
elif isinstance(self.license, list): | |||
self.license = self.license[0].replace(' ', '-') |
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.
@shaneallcroft this hackyness needs to be killed with fire. This just removes the list entirely and replaces it with its first element.
Therefore, this line and the one that immediately follows should be removed.
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.
You got it! 👍 🔥 🔥
…info for license as list
ret += ' ' | ||
first = False | ||
ret += lic | ||
ret += 'LICENSE = "' + ' & '.join(self.license) |
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.
This is very close. Make sure to call the getlicense
function on each entry of the list! Something like the following would probably do the trick.
' & '.join([getlicense(l) for l in self.license])
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 yeah, that makes sense, cause the hackyness is gone so every item needs to be taken into account 👍 im on it 👍
@@ -134,18 +136,8 @@ def get_recipe_text(self, distributor, license_text, die_msg=None): | |||
self.license = self.license.replace(' ', '-') | |||
ret += 'LICENSE = "' + self.license + '"\n' |
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.
@shaneallcroft You might update this one as well.
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 heck yeah! I'm on it 👍
:( wait i messed it up |
It happens. Looks like you just missed an import for the |
…ply with the 79 chararcter limit
I forgot the underscore i think just a sec |
First try 😎 fixing the other thing now almost for got |
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.
You're very close.
self.license = self.license.replace(' ', '-') | ||
ret += 'LICENSE = "' + self.license + '"\n' | ||
ret += 'LICENSE = "' | ||
ret += ' & '.join([get_license(l) for l in self.license]) + '"\n' | ||
elif isinstance(self.license, list): |
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.
So I don't think you need to use a join when self.license
is a string. I think this line could just be
ret += 'LICENSE = "%s"\n' % getlicense(self.license)
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.
Ahhh, i didnt see that '[0]' on the end of line 136, thought it was just being made into a list there so i treated it the same, as when it was a list my b!
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.
Lgtm. I'll let @tfoote merge.
Awesome!!! Thank you so much! : ) |
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.
@shaneallcroft Thanks for iterating on this. It looks good to me now.
(also woot #100 -- that's pretty cool) |
…nse ToDo (ros-infrastructure#100) Replicated copyright change to open imbedded, and resolved multi license to do
#100 Hooray!!!!!!