Skip to content
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 new post check for share folder #675

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

JackBoosY
Copy link
Contributor

Check for non-lowercase share folder.

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

@ras0219-msft For regression, should I split the message to fix it?

* Move FolderNameMismatchedCasing to messages.h / messages.cpp
* Remove trailing \n
* Regenerate jsons
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I fixed the build failures and I think the change is overall good. Because it adds a new post-build lint we need to pre-run it on the catalog to double check that the world won't break when deployed. @ras0219-msft Can you confirm you're OK with this now and I can kick that off?

@BillyONeal
Copy link
Member

@ras0219-msft For regression, should I split the message to fix it?

The error was complaining that the localized message ended with \n, thus being likely to create a bug that was present here where it was passed to println doubling the terminal \ns. This check was added in #553

@ras0219-msft
Copy link
Contributor

LGTM. @BillyONeal if you could kick off the test that would be great!

@dg0yt
Copy link
Contributor

dg0yt commented Sep 17, 2022

Well, you can just use the file lists from CI... I find

  • more than 7000 installed files matching :/share/[A-Z]
  • more then 600 CMake configs, by :/share/[A-Z].+/.*onfig.cmake
  • more then 60 port dirs, by :/share/[A-Z].+/vcpkg_abi

@autoantwort
Copy link
Contributor

Why folders in the share folder should be always lowercase?

@BillyONeal
Copy link
Member

Well, you can just use the file lists from CI... I find

  • more than 7000 installed files matching :/share/[A-Z]
  • more then 600 CMake configs, by :/share/[A-Z].+/.*onfig.cmake
  • more then 60 port dirs, by :/share/[A-Z].+/vcpkg_abi

Good point!

Why folders in the share folder should be always lowercase?

The name vcpkg creates is always, for example, share/${PORT}/vcpkg_abi_info.txt, and ${PORT} is always lowercase. When there are capital ones, it means that users will get different behavior on case sensitive vs. case insensitive file systems, which has been the source of a lot of broken behavior. (like 'works on linux but not windows because the directory names differed only in case')

@BillyONeal
Copy link
Member

BillyONeal commented Sep 19, 2022

Why folders in the share folder should be always lowercase?

Example we want the new check to block microsoft/vcpkg#26284 (comment)

@dg0yt
Copy link
Contributor

dg0yt commented Sep 23, 2022

Port ogre installs resources to share/OGRE. To be patched?

@dg0yt
Copy link
Contributor

dg0yt commented Sep 26, 2022

Port ogre installs resources to share/OGRE. To be patched?

This mirrors the headers in include/OGRE and the plugins in lib/OGRE.

…ev/jack/new-lint-postbuild-check

# Conflicts:
#	include/vcpkg/base/messages.h
#	locales/messages.en.json
#	locales/messages.json
#	src/vcpkg/base/messages.cpp
@ras0219-msft
Copy link
Contributor

Port ogre installs resources to share/OGRE. To be patched?

That's a great point. We do need a policy setting to control the check. Then, we should evaluate whether ogre should disable the policy or whether patching is appropriate. I don't think that matching lib/OGRE/ or include/OGRE/ is by itself sufficient, but if there is an expectation of hardcoded paths containing share/OGRE/ then we may need to simply accept the differences per host platform.

@ras0219-msft
Copy link
Contributor

@JackBoosY Once again, thank you for all your hard work on vcpkg! Would you like to continue this PR or should we take ownership and bring it to completion?

@JackBoosY
Copy link
Contributor Author

@ras0219-msft I will continue this PR.

@JackBoosY
Copy link
Contributor Author

Looks good now.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 13, 2023

It still needs a policy to disable it, and changes to app. 60 ports.

@JackBoosY
Copy link
Contributor Author

@ras0219-msft @BillyONeal Any feedback?

@dg0yt
Copy link
Contributor

dg0yt commented Feb 14, 2023

The feedback is only four days old (#675 (comment))

We do need a policy setting to control the check.

@JackBoosY
Copy link
Contributor Author

The feedback is only four days old (#675 (comment))

We do need a policy setting to control the check.

Ah I'm blind...

@JackBoosY
Copy link
Contributor Author

So what about the 60+ ports / port OGRE?

@dg0yt
Copy link
Contributor

dg0yt commented Feb 24, 2023

FTR at least to open PRs add new ports with mixed case directories via vpckg_cmake_config_fixup(PACKAGE_NAME <Mixed>). Maybe it would be a good idea to start with improving this function: Convert input PACKAGE_NAME to lower case directory name where feasible.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented Feb 28, 2023

FTR at least to open PRs add new ports with mixed case directories via vpckg_cmake_config_fixup(PACKAGE_NAME <Mixed>). Maybe it would be a good idea to start with improving this function: Convert input PACKAGE_NAME to lower case directory name where feasible.

Will do that after microsoft/vcpkg#29880 microsoft/vcpkg#29882 microsoft/vcpkg#29883 microsoft/vcpkg#29884 and microsoft/vcpkg#29918 are merged.

@JackBoosY
Copy link
Contributor Author

Depends on microsoft/vcpkg#31053

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants