CROSSTOOLS wrapped_clang: handle spaces in paths

[ob]

When bazel calls wrapped_clang, it single-quotes all arguments. However
it passes flags with arguments quoted as a whole. That is, wrapped_clang
will be called with arguments like these:

wrapped_clang '-isysroot /a/path/with spaces' '/a/file with spaces.m'

Before this commit, wrapped_clang was blindly splitting on space and
calling clang with invalid arguments. Now it only splits on the first
space, and only if the argument starts with '-'.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[ob]

I signed in!

[googlebot]

CLAs look good, thanks!

[sergiocampama]

what if it's passed as -isysroota/path/that might have/spaces? or with the = syntax?

[ob]

Good point. I'm assuming this is called by Bazel right? I grepped for isysroot and it seems Bazel does use a single space:

tools/osx/crosstool/CROSSTOOL.tpl:        flag: "-isysroot %{sdk_dir}"

for example. Is it the intent to have it be general purpose and support all the different syntaxes? Note that this might be really hard, for instance in the -isysroota/path case, how do you know where the flag ends and the path starts without knowing all of clang's flags?

Also note the previous version would fail in similar or worse ways with either of these other syntaxes.

[ob]

Thinking a bit more about this change I'm wondering if it wouldn't be better to change Bazel to not combine multiple arguments into one. Is there a reason it does: wrapped_tool '-isysroot path instead of wrapped_tool '-isysroot' 'path'? That way the wrapped tools wouldn't have to parse arguments which is fragile...

[ob]

As an update, I have found a case where this change breaks. When trying to build a sample app, I found Bazel calling the wrapped_clang tool with the following argument, which would need to be split into multiple arguments.

'-Xlinker -add_ast_path -Xlinker bazel-out/ios-x86_64-min9.0-applebin_ios-ios_x86_64-fastbuild/genfiles/sample_appBinary/_objs/sample_appBinary.swiftmodule'

I think what I said in my previous comment is the right way forward.

[ulfjack]

+1

[ob]

Okay, I played a little with this but I'm stuck. The way the flags end up with spaces in them is because in tools/osx/crosstool/CROSSTOOL.tpl they are specified as:

       flag_group {
         flag: "-isysroot %{sdk_dir}"
       }

I've verified that if I remove that space and make the flag two entries, like so:

       flag_group {
         flag: "-isysroot"
         flag: "%{sdk_dir}"
       }

The problem is solved. However that file says at the beginning:

# This file was auto-generated by a script maintained internally by
# Google.

Which makes me think that changing it directly is a bad idea. Also, there are other instances of spaces in the flags, but there is some kind of iteration going on, e.g.

      flag_group {
        flag: "-weak_framework %{weak_framework_names}"
        iterate_over: "weak_framework_names"
      }

I am not familiar with the language specification for this file so I don't know how that could be turned into multiple flag entries that are paired with their arguments.

[ob]

Those last commits kind of work for me but I'm not sure whether that iterate_over iterates over the flag_group or single flags... or whether this needs to be changed in some internal Google script.

[ulfjack]

tools/osx/crosstool/CROSSTOOL.tpl changes require a change in an internal Google script, correct. Is the patch in this CL the resulting file we need to get?

[ulfjack]

I have a change to update the script that I'm about to send out. It should get referenced here once it's merged.

[ob]

Yeah, the changes to CROSSTOOL.tpl are what I'm testing with. Modulo my previous comments about my lack of knowledge of tpl format and whether it expands flag_groups as I think it expands them, then yeah.

Note that I also removed the parsing from the clang wrapper.

Thank you for taking a look at this.

[ulfjack]

My change was merged. Please rebase and let me know whether you can get it to work now.

[ob]

@ulfjack I tested this with my workspace and the test case that I had added before. Note that I removed splitting on spaces that wrapped_clang used to do.

[ulfjack]

Generally looks good. I triggered the CI.

[ob]

I should probably fix the comments before you merge and reference the commit you did that fixes the caller side...

[ob]

Ok, pushed a NFC with better comments... sorry I didn't do the comments right the first time. I thought the CI was triggered automatically.

[ulfjack]

@ob, you can't trigger CI yourself. Security told us we're required to do a code review first.

[ob]

@ulfjack that makes sense... in a way triggering a CI on unreviewed code changes can be seen as a remote execution attack ;-)

Could you please trigger a CI run again?

[ulfjack]

Triggered.

[ob]

Hmm... the test failure seems unrelated to my changes... FWIW, //src/test/java/com/google/devtools/build/lib:remote-tests passes on my machine.

[ulfjack]

Yeah, the test is flaky - I just disabled it earlier today. I'm going to merge this PR now.