-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
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.
I merely confirmed that image/build-wheel.sh
is placing the files into the correct location in the wheel. That's sufficient for me as product owner. I'll trust y'all confirm that this PR collects the sufficient list of files.
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.
Besides note about lz4 below, confirmed that all of the expected license files are copied over.
@@ -12,3 +12,7 @@ ExternalProject_Add(lz4 | |||
PREFIX=${CMAKE_INSTALL_PREFIX} | |||
install | |||
) | |||
|
|||
extract_license(lz4 | |||
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.
In addition to the top-level LICENSE file, do we also need the lib/LICENSE file?
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.
Good catch, thank you! Actually, I don't think we need the top-level LICENSE
; #18 was right.
(I think this is the one instance in #18 when the source and destination paths are not the same; I missed that when copying over that logic. If you know of others, please let me know, as they are likely also wrong.)
Is having the LICENSE
in pydrake/doc/lz4/lib
going to be an issue?
(I'll update once my local test build finishes.)
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 the only instance when the source and destination were different. Personally, I don't mind the extra subdirectory
@jwnimmer-tri, it would probably be good if you could also sanity-check the various comments about what isn't copied. |
Add logic to gather license files of the various dependencies, as needed, into a common directory. Update wheel build to include these in the wheel. Co-authored-by: Betsy McPhail <[email protected]>
86bcb08
to
fb299fe
Compare
They seem fine. I agree that build-time-only doesn't need notice. Personally I would have probably still included in the Zlib-style notices just as courtesy credit to those authors, but I don't think its wrong to leave it out. |
I'd be happy to revisit/reconsider. A significant reason for omitting them, at least for tinyxml2, is that the version we're currently using doesn't have a proper license file. That should be fixed if we move to a newer version, so I suppose the questions are a) do we want to do so just for the license (which is not strictly necessary), and b) do we want to do it in this PR, or as a follow-up? |
My suggestion would be to change the tinyxml2 comment along the lines of "Once we're using a version of tinyxml2 that has a license file, we should install it as a courtesy (even though we are no legally bound to)." |
Add logic to gather license files of the various dependencies, as needed, into a common directory. Update wheel build to include these in the wheel.
Supersedes #18.