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

ninja: Use platform dependent quote instead of shlex.quote() in gcc_rsp_quote() #12647

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

byteblob42
Copy link

See also #12643

@dcbaker
Copy link
Member

dcbaker commented Dec 21, 2023

That listing error looks relevant

@byteblob42
Copy link
Author

byteblob42 commented Dec 21, 2023

That listing error looks relevant

Ok, so just purge the import yeah? Should i just make an additional commit or what would you like?

@byteblob42
Copy link
Author

I'll be gone until the middle of the first january week, feel free to close this if you wish, I'll make a new PR when I'm back in that case.

@byteblob42
Copy link
Author

I have ammended the commit.

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

LGTM

@eli-schwartz
Copy link
Member

Why is the location of the function moved around in the file?

@dcbaker
Copy link
Member

dcbaker commented Jan 8, 2024

quote_func is defined until after where the gcc_rsp function was originally defined

@eli-schwartz
Copy link
Member

eli-schwartz commented Jan 8, 2024

The order doesn't strictly matter since quote_func() is only used after the function is defined. That being said I don't strongly care one way or another.

It's a bit neater to follow the logic without moving it, but not a big deal.

@dcbaker dcbaker merged commit ce2db13 into mesonbuild:master Jan 9, 2024
34 checks passed
@jon-turney
Copy link
Member

I am very sceptical that this change is correct.

The unstatated assumption behind #12643 is that it's possible to write a ninja.build which simultaneously satisfies both:

  1. response file contents are correctly quoted for the tools which use them
  2. response file contents are quoted in the "platform native" style, which consumers of the ninja compdb are expecting

Even on pure Unix, this isn't 100% right, because things like '\\' have a different meaning under gcc response file and shell quoting rules. but I guess since this solves the OPs problem, it all happens to work in most cases.

It might be that meson just doesn't (currently) generate response files with problematic contents, but I suspect this is probably going to break down where these quoting styles are just incompatible (so perhaps something involving quoting a " character).

(As a thought experiment, maybe consider what happens if I've written some bonkers tool which defines it's response file quoting style involves using some bizaare non-standard quoting mechanism. I can write a ninja.build using that without problems, but naive expectations about the contents of the compdb are going to be broken...)

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