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

Unwrap clang using clang version #40

Closed
wants to merge 1 commit into from

Conversation

jun-sheaf
Copy link

This PR obtains clang through a given clang wrapper by calling its --version and parsing the installation directory. In particular, this should be universal with any "nice" toolchain that mimic Bazel's clang wrapping.

This PR obtains clang through a given clang wrapper by calling its `--version` and parsing the installation directory. In particular, this should be universal with any "nice" toolchain that mimic Bazel's clang wrapping.
@cpsauer
Copy link
Contributor

cpsauer commented Apr 3, 2022

Oh, nice. That's a clever unwrapping trick.

Does this work with the Apple wrapper, though? I'd think the wrapper might have already crashed without the environment variables before it even gets to invoke clang with --version? (Haven't gotten a chance to test yet; trying to help get Windows support landed.)


If that is a problem, could we solve your case (as in the other PR) by only applying the current unwrapping if the compiler ends in .sh? You still haven't really explained what the original problem you were running into was, but I think it's that you're using a custom compiler on Apple platforms, where you still need the __BAZEL_XCODE_ expanded? :)

If that's not a problem: We probably shouldn't do it both in _all_platforms_patch and _apple_platform_patch. That seems like double coverage. And clangd can introspect non-crashing wrappers with --query_driver just fine.

@jun-sheaf
Copy link
Author

There was some misunderstanding on my end. This does work on the Apple wrapper, but there is that slight problem of double coverage. Considering wrapped_clang is extensionless however, it should be fine to just leave it as is. Also, this PR only affects wrappers ending in .sh which to my knowledge always crash. Are there others?

@cpsauer
Copy link
Contributor

cpsauer commented Apr 5, 2022

Hey, yeah, so I just tested it, and this doesn't seem to work with the default Apple wrapper. Running the wrapper with --version crashes because it's missing the required environment variables, as hypothesized above.

❯ ./external/local_config_cc/wrapped_clang_pp --version
Error: DEVELOPER_DIR not set.
[1]    97761 abort      ./external/local_config_cc/wrapped_clang_pp --version

[Note to self: If merged, would need something parallel for Windows.]

FWIW there are .sh wrappers, like the grailbio LLVM ones, that work fine, last I checked, at least with --query-driver passed. Wouldn't be upset if they were unwrapped, but we don't need to. Indeed, the only ones I know of that don't work are the Apple default wrappers.

My mistake on the apple wrapped_clang and wrapped_clang_pp not having .sh extensions. No double coverage, but it does (at least) break things for a default macOS setup.

@cpsauer
Copy link
Contributor

cpsauer commented Apr 5, 2022

Could you explain a little more to me about what issue we're trying to solve here? Maybe with an example?
[I've asked and guessed a couple times on the PR and the last, but I still don't know exactly.]

@cpsauer
Copy link
Contributor

cpsauer commented Apr 6, 2022

^ Please do still answer, but would just adding

if compile_args[0].endswith(('/wrapped_clang', '/wrapped_clang_pp')):
before
compile_args[0] = _get_apple_active_clang()
solve your issue?

@jun-sheaf
Copy link
Author

The grailio wrappers were not working. I've moved development to containerized Linux, so all of this is not really necessary for me anymore, however it probably makes sense to use the version for unwrapping.

@cpsauer
Copy link
Contributor

cpsauer commented Apr 6, 2022

It would still be great to understand (and fix) whatever the underlying issue was! Both for you and for others.
Could I ask you to answer the questions above? (It feel like we're talking past each other a bit.)

[I just ran a quick test w/ the grailbio toolchain and couldn't coax out an issue--so I need your help. We left the compiler wrapped, but --query-driver (from the README) caused clangd to correctly introspect the default includes.]

@jun-sheaf
Copy link
Author

Ah ha! I did not know about the --query-driver option (I was using this before you added this change in the README).

It would still be great to understand (and fix) whatever the underlying issue was!
As stated previously, the grailio wrappers were not working, but this was before you added the --query-driver instructions.

@jun-sheaf jun-sheaf closed this Apr 6, 2022
@cpsauer
Copy link
Contributor

cpsauer commented Apr 6, 2022

Oh! Great! Glad that solved it for you :)

@cpsauer
Copy link
Contributor

cpsauer commented Apr 6, 2022

All that said, if we ever do want/need to unwrap everything, I'm going straight to your --version based fix--at least outside of the default macOS wrapper. Thanks for a clever solution that'll we'll have in our tool belts if we ever need it.

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.

None yet

2 participants