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

CMakeLists.txt: Make generate-message-map target always available #854

Conversation

autoantwort
Copy link
Contributor

@BillyONeal
Copy link
Member

As I mentioned in the other comment, I don't believe it is correct to create a CMake target that is not functional. You need artifacts development to be on to produce the localized messages for artifacts.

@autoantwort
Copy link
Contributor Author

Why? The file ce/ce/locales/messages.json is checked in and is not changed if I do only non artifacts development.

@autoantwort
Copy link
Contributor Author

I tested this locally and it works totally fine. I don't get where it is supposed to break?

@BillyONeal
Copy link
Member

@JavierMatosD Did you hook up the thing which embeds the artifacts messages into the same json or did I miss something?

@autoantwort
Copy link
Contributor Author

Did you hook up the thing which embeds the artifacts messages into the same json or did I miss something?

No this is still the case.
x-generate-default-message-map reads ce/ce/locales/messages.json and combines it with the messages in the vcpkg-tool code and writes the result to locales/messages.json.

But when I only change the messages in the vcpkg-tool code I can run x-generate-default-message-map to get the updated locales/messages.json without touching artifacts development. So why should the cmake custom target unable to do the exact same?

@JavierMatosD
Copy link
Contributor

@autoantwort and @BillyONeal I did not intend to check in the artifacts JSON file. That is why it is guarded by VCPKG_ARTIFACTS_DEVELOPMENT. However, by keeping it checked in, users can contribute to localization without the need for additional tools like Node, NPM, or Rush.

@JavierMatosD JavierMatosD merged commit 23956b0 into microsoft:main Jan 17, 2023
@autoantwort autoantwort deleted the feature/make-generate-message-map-target-always-available branch January 17, 2023 20:56
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.

3 participants