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

pkgconfig post-build-check: Detect more problems, fix output and add e2e tests #865

Merged

Conversation

autoantwort
Copy link
Contributor

Currently not all wrong placed files are detected. Also the output was broken after the localization. Added e2e tests to verify the new check

@autoantwort autoantwort force-pushed the feature/fix-pkgconfig-postbuildcheck branch from 9d4a79d to 978b65d Compare January 23, 2023 15:18
@autoantwort autoantwort force-pushed the feature/fix-pkgconfig-postbuildcheck branch from 978b65d to 8bb5ebd Compare January 23, 2023 18:37
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.

Looks good except for the one nitpick. Thanks!

(),
"Only the 'empty directories left by the above renames' part should be translated",
"vcpkg_fixup_pkgconfig()\nfile(REMOVE_RECURSE empty directories left by the above renames)");
DECLARE_MESSAGE(PortBugRemoveEmptyDirs, (), "", "empty directories left by the above renames");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is actually an improvement: it is important for translators to be able to see the context for the message, and it isn't in general safe to just combine things after the fact.

I think the correct fix is just to delete the:

msg::write_unlocalized_text_to_stdout(Color::warning, " vcpkg_fixup_pkgconfig()\nfile(REMOVE_RECURSE ");

line instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now file(REMOVE_RECURSE empty directories left by the above renames)

@BillyONeal BillyONeal merged commit 9a828f5 into microsoft:main Feb 11, 2023
@BillyONeal
Copy link
Member

Thanks for the fixes!

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

Successfully merging this pull request may close these issues.

2 participants