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

Fixes for finding png crush executable #1484

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

colincornaby
Copy link
Contributor

I'm not sure why this is required or how the fix improves things - but the part of the CMake script that finds pngcrush has been failing on some of my Macs. I'm not sure why - the Mac that I am seeing this on is running the latest version of CMake.

These changes to the script seem to make things work.

If anyone has a CMake perspective on why this is going on or why the fix works - that would be appreciated.

@@ -29,7 +29,7 @@ cmake_dependent_option(RESOURCE_BRUTE "Allow pngcrush brute-force optimization"
if(PLASMA_BUILD_RESOURCE_DAT)
set(external_DAT "${CMAKE_CURRENT_BINARY_DIR}/resource.dat")
if(RESOURCE_OPTIMIZE)
string(APPEND OPTIMIZE_ARGUMENT "--pngcrush \"${PNGCRUSH_EXECUTABLE}\" ")
string(APPEND OPTIMIZE_ARGUMENT "--pngcrush=\"${PNGCRUSH_EXECUTABLE}\"")
Copy link
Member

Choose a reason for hiding this comment

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

We need to keep the space at the end, because it appends other arguments to the string without a leading space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that causes things to fail again - but lemme double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does actually seem to work on this machine even with the space. I'll make the edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take it back. The space is once again not working.

Showing All Messages
FileNotFoundError: [Errno 2] No such file or directory: '/Applications/Xcode.app/Contents/Developer/usr/bin/pngcrush '

See how the space is incorporated into the file path?

As I was researching this I found some information that CMake does not do simple string work very well and will try to automatically escape things or encapsulate them in quotes. That seems like this is what is happening here but I don't know enough to make a good guess as to what the best fix is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you print out the value of PNGCRUSH_EXECUTABLE from inside the CMake script to check if perhaps the space is already there in the variable and not caused by the argument building? In my experience, CMake usually handles string quoting/escaping very well - so if the argument passed to Python has a trailing space, that probably means that there's an actual space at the end of the variable value.

I wonder if it's a problem that OPTIMIZE_ARGUMENT is a single string of space-separated arguments, not a proper CMake list. Does the behavior change if you replace string(APPEND ...) with list(APPEND ...) and make sure there are no spaces inside the quoted strings? (For that to work, you might have to pass COMMAND_EXPAND_LISTS to add_custom_command, but I'm not sure...)

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

I wonder if it's a problem that OPTIMIZE_ARGUMENT is a single string of space-separated arguments, not a proper CMake list.

This is correct. The problem here is that we're crossing wires a bit. CMake really only has one data type - string. Lists, booleans, and such are really just specially formatted strings. The problem here is that we're mixing a command line argument string into a list of arguments.

To illustrate, this is a string with the contents Hello World

set(hello "Hello World")

These are a list with the items Hello and World

set(hello Hello World)
set(hello Hello;World)

when printed, this will appear as Hello;World.

The variable OPTIMIZE_ARGUMENT is being reproduced unquoted, which is a hint that we want to retain magical datatypes eg lists. Unfortunately, the code populating the variable is using string functions, which doesn't make the semicolon separated list. So, our arguments wind up all goofy python.exe;makeres.py;--render;--pagkage;--pngcrush pngcrush.exe --brute;-i;/external;-w;/bin;-o;/bin. Using list commands will properly populate OPTIMIZE_ARGUMENT as a list.

Sources/Plasma/Apps/plClient/CMakeLists.txt Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Adam Johnson <AdamJohnso@gmail.com>
@colincornaby
Copy link
Contributor Author

Proposed changes seem to work fine on my Mac.

@Hoikas Hoikas merged commit 46966e4 into H-uru:master Sep 12, 2023
14 checks passed
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.

4 participants