Skip to content

[vcpkg postcheck] Check the folder name of the generated cmake file#12604

Closed
JackBoosY wants to merge 15 commits intomicrosoft:masterfrom
JackBoosY:dev/jack/check_cmake_file_directory
Closed

[vcpkg postcheck] Check the folder name of the generated cmake file#12604
JackBoosY wants to merge 15 commits intomicrosoft:masterfrom
JackBoosY:dev/jack/check_cmake_file_directory

Conversation

@JackBoosY
Copy link
Copy Markdown
Contributor

@JackBoosY JackBoosY commented Jul 28, 2020

Since find_package is case-sensitive in *nix, check after the build whether the name of the generated cmake file matches the name of the folder where it is located.

@JackBoosY JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal labels Jul 28, 2020
@JackBoosY JackBoosY requested a review from strega-nil July 28, 2020 00:47
@JackBoosY JackBoosY changed the title [vcpkg] Check the folder name of the generated cmake file [vcpkg postcheck] Check the folder name of the generated cmake file Jul 28, 2020
@JackBoosY JackBoosY marked this pull request as ready for review August 5, 2020 02:50
@JackBoosY JackBoosY requested a review from PhoebeHui August 10, 2020 02:37
@PhoebeHui PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Aug 10, 2020
Copy link
Copy Markdown
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I see some changes that need to be done so that it follows the style of vcpkg, and also some better ways to do the things (using Strings::ends_with instead of .find, for example). Additionally, we need to use fs::u8string instead of .string(), to avoid locale-specificity.

Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
Comment thread toolsrc/src/vcpkg/postbuildlint.cpp Outdated
@strega-nil
Copy link
Copy Markdown
Contributor

@JackBoosY could you do the suggestions?

@JackBoosY
Copy link
Copy Markdown
Contributor Author

@strega-nil Sorry, I forgot it. Done.

@strega-nil
Copy link
Copy Markdown
Contributor

strega-nil commented Aug 14, 2020

Hrmm, now that I'm looking at this, this check seems wrong? Check out the find_package documentation:

In all cases the <name> is treated as case-insensitive and corresponds to any of the names specified (<PackageName> or names given by NAMES).

@JackBoosY
Copy link
Copy Markdown
Contributor Author

JackBoosY commented Aug 18, 2020

@strega-nil Sadly, it does affect us to get the correct *-config.cmake.
See #12592 changes.
vcpkg sets the search path SEARCHNAME_DIR to installed/TRIPLET/share/PORT_NAME, so that find_package can find the correct cmake file under this path, and PORT_NAME is case sensitive in Linux.

@BillyONeal
Copy link
Copy Markdown
Member

This doesn't build cleanly anymore due to the u8path thing.

@BillyONeal BillyONeal added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Aug 18, 2020
@JackBoosY
Copy link
Copy Markdown
Contributor Author

@BillyONeal More details?

@BillyONeal
Copy link
Copy Markdown
Member

I assume the Arrow 'regression' is what you are trying to look for? :)

auto filename = fs::u8string(file_path.filename());
if (Strings::ends_with(filename, UPPER_CONFIG))
{
config_prefix = filename.substr(0, filename.size() - UPPER_CONFIG.size());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we also check for "CoNFiG.cmAkE" (And possibly fail the results if any such things are present?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, generally, the file names generated by cmake are fixed.

Comment on lines +457 to +458
cmake_file.filename().string().c_str(),
cmake_file.parent_path().string().c_str(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These probably need to use some form of u8something rather than string()

@ras0219
Copy link
Copy Markdown
Contributor

ras0219 commented Aug 20, 2020

vcpkg sets the search path SEARCHNAME_DIR to installed/TRIPLET/share/PORT_NAME

Can you link where this happens?

Copy link
Copy Markdown
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I would like to see the bug this fixes.

@JackBoosY
Copy link
Copy Markdown
Contributor Author

@ras0219 Maybe I misunderstood this point, but it did cause some problems.

@JackBoosY
Copy link
Copy Markdown
Contributor Author

@strega-nil Example #12997 #12592 etc.

@JackBoosY
Copy link
Copy Markdown
Contributor Author

JackBoosY commented Aug 21, 2020

According to the official document, I think this PR is no longer needed.
We only need to ensure that the folder name is lowercase (already do that).

@JackBoosY JackBoosY closed this Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants