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

curl-config: quote directories with potential space #9253

Closed
wants to merge 1 commit into from

Conversation

orgads
Copy link
Contributor

@orgads orgads commented Aug 5, 2022

On Windows (at least with CMake), the default prefix is
C:/Program Files (x86)/CURL.

curl-config.in Show resolved Hide resolved
@bagder bagder added the script label Aug 6, 2022
@bagder
Copy link
Member

bagder commented Aug 6, 2022

I don't think changing the format of that data is going to make existing users of this happy. I would prefer to have users opt-in to have spaces treated correctly. Maybe with a new switch. Maybe we should then do an overhaul of all outputs in there and see if they could be improved for this aspect?

@orgads orgads force-pushed the curl-config-quote branch 2 times, most recently from 0f66553 to 6898b9b Compare August 7, 2022 04:13
@orgads
Copy link
Contributor Author

orgads commented Aug 7, 2022

I don't think changing the format of that data is going to make existing users of this happy. I would prefer to have users opt-in to have spaces treated correctly. Maybe with a new switch. Maybe we should then do an overhaul of all outputs in there and see if they could be improved for this aspect?

The current script has invalid syntax when there are spaces in prefix or libdir. I only fixed those that prevent it from running.

Every time I run runtests.pl, I get a warning:

../curl-config: line 26: syntax error near unexpected token `('
../curl-config: line 26: `prefix=C:/Program Files (x86)/CURL'

And a similar one for line 177.

@bagder
Copy link
Member

bagder commented Aug 7, 2022

I only fixed those that prevent it from running.

Right, that was the line 26 edit. The line 177 one is different. It adds double quotes to the output and assumes that the user of the option will handle them properly. I presume it breaks if the path itself contains double quotes.

@orgads
Copy link
Contributor Author

orgads commented Aug 7, 2022

I added the internal quotes following your previous review. The output looks reasonable to me.

Do you have a better alternative? Do you prefer changing the default prefix to something without spaces maybe?

@bagder
Copy link
Member

bagder commented Aug 7, 2022

I added the internal quotes following your previous review. The output looks reasonable to me.

Yes, it is probably a reasonable format if we would create it as a new format today. But we aren't. We change an existing output that presumably existing tools/scripts already parse and use. Keeping those existing users is an important property. So I would like the change to be done with minimum risk of breakage.

On Windows (at least with CMake), the default prefix is
C:/Program Files (x86)/CURL.
@orgads orgads force-pushed the curl-config-quote branch from 6898b9b to a0aec16 Compare August 8, 2022 11:52
@orgads
Copy link
Contributor Author

orgads commented Aug 8, 2022

Ok, reverted to the first revision (no extra quotes in the output).

@bagder bagder closed this in 3adc9f3 Aug 10, 2022
@bagder
Copy link
Member

bagder commented Aug 10, 2022

Thanks!

@orgads orgads deleted the curl-config-quote branch August 10, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants