Conversation
|
This pr MUST NOT CONTAIN any improvement nor any update to any port. It is just a review of portfiles to make them adhere better to CMake standards and vcpkg's best practices. |
|
This hypothesis of course means that any regression is really unexpected, but will be fixed of course, with the least impact possible on the port itself. |
ports/lcm/portfile.cmake
Outdated
| endif() | ||
|
|
||
| vcpkg_copy_tool_dependencies(${CURRENT_PACKAGES_DIR}/tools/lcm) | ||
| vcpkg_copy_tool_dependencies(${CURRENT_PACKAGES_DIR}/tools/${PORT}) |
There was a problem hiding this comment.
looks like the arguments is not even required if vcpkg_copy_tool_dependencies has a reasonable default
There was a problem hiding this comment.
which I didn't even check tbh
ports/theia/portfile.cmake
Outdated
| file(COPY ${SOURCE_PATH}/license.txt DESTINATION ${CURRENT_PACKAGES_DIR}/share/theia) | ||
| file(RENAME ${CURRENT_PACKAGES_DIR}/share/theia/license.txt ${CURRENT_PACKAGES_DIR}/share/theia/copyright) | ||
| file(COPY ${SOURCE_PATH}/data/camera_sensor_database_license.txt DESTINATION ${CURRENT_PACKAGES_DIR}/share/theia) | ||
| file(INSTALL ${SOURCE_PATH}/license.txt DESTINATION ${CURRENT_PACKAGES_DIR}/share/${PORT} RENAME copyright) |
There was a problem hiding this comment.
after reaching this file this should probably a function like: vcpkg_install_copyright or vcpkg_install_license just to make it shorter. The only thing required is the source name/location of the file to copy everything else is always the same.
There was a problem hiding this comment.
I was thinking about the same. Maybe for now just putting the license at the end with the same signature everywhere is enough. It may become very easy later to just replace it with a custom vcpkg function
There was a problem hiding this comment.
we've been using configure_file() in modern ports for this purpose:
configure_file(${SOURCE_PATH}/license.txt ${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright COPYONLY)There was a problem hiding this comment.
Ok, even if for me configure_file means that the input file will be processed to become the output file. Why? To simplify the signature and have only “to” and “from”?
There was a problem hiding this comment.
configure_file() feels like a hack.
just introduce vcpkg_install_license or vcpkg_copy_license (or name it whatever you find appropiate) which only requires 1 argument which is the relative location of the license to the source_path. Gets rid of all those noise. It could also be made so that it automatically searches for common license filenames.
I mean how many different license names are there in the 1000 ports? 4 to 6 ? We could even call the function searching for the license vcpkg_find_license (just like find_library ;))
${SOURCE_PATH}/license.txt ${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright COPYONLY
is just obvious and unnecessary noise.
There was a problem hiding this comment.
vcpkg_install_copyright is a good start, because in the long run copyright should do more checking
Only 4 groups are required:
- public domain (no license)
- MIT like (no copyleft, can be re licensed)
- LGPL like (no copyleft, but needs dll linkage)
- GPL like (copyleft, infects, no way out)
Just a few suggestions,
- Copyright should also be available in the port file, and show up in the search list.
- Mark problematic ports (Libiconf and liblzma are my favorite enemies currently ;) )
- Need a way to specify license impact per port feature
- Care if using a license with a higher number
|
@Neumann-A would you like to join the effort? |
|
ok no problem |
|
I am seeing this on Linux in the CI system with this PR: |
|
It became 644 (permissions) due to using Windows’ git for committing... whoops! |
|
Some reviews:
|
I'd like to advise that someone with access adds a check which ensures the appropriate executable status of all .sh files within the vcpkg tree. Doing so using the CI bot should go some way towards preventing future removal of executable mode status from files which require said status. |
|
/azp run |
|
Pull request contains merge conflicts. |
|
If this is a dead PR we should close it. |
|
@cbezault I will rebase this PR very soon to continue the work. Sorry for the waiting |
cenit
force-pushed
the
dev/cenit/modernize
branch
from
43eb854 to
4cffadc
Compare
13 participants
Work in progress towards the idea of using a FATAL_ERROR for unmerged patches (related to updates that broke them without any notice and has been very dangerous in the past)