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

Generate ccache wrapper scripts instead of symlink #39

Merged
merged 7 commits into from
Jun 11, 2022

Conversation

chenhao94
Copy link
Collaborator

Tested with Bazel 5.2. It is a workaround for issue
bazelbuild/rules_cc#130

scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
@xkszltl
Copy link
Collaborator

xkszltl commented Jun 8, 2022

  • What's the bug previously?
  • Is it fixed in upstream?
  • If it's symlink-related, can we use the same CC approach with scripts instead of symlink?
  • Put some comments in place about what's working and what's not.

@chenhao94
Copy link
Collaborator Author

  • What's the bug previously?

ccache symlink to clang will be derefed by Bazel

  • Is it fixed in upstream?

No.

  • If it's symlink-related, can we use the same CC approach with scripts instead of symlink?

That is exactly what this PR does.

  • Put some comments in place about what's working and what's not.

Will do

@xkszltl
Copy link
Collaborator

xkszltl commented Jun 9, 2022

That is exactly what this PR does.

I mean give it CC without PATH

scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/codebase_utils.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
@chenhao94
Copy link
Collaborator Author

That is exactly what this PR does.

I mean give it CC without PATH

No, because Bazel does not like full path. I # think the point is, you either use installed compilers (those can be find by which), or define a Bazel toolchain. The latter one seems to be a preferred way.

@xkszltl
Copy link
Collaborator

xkszltl commented Jun 9, 2022

... or define a Bazel toolchain. The latter one seems to be a preferred way.

Not sure what does toolchain looks like here, but it should still use the full path.
Problem with PATH is you're not suppose to expose this to whatever Bazel's calling.
They may not like to PATH required by PATH.
PATH is used by everything, but things like CC has a much narrower context.

@chenhao94
Copy link
Collaborator Author

... or define a Bazel toolchain. The latter one seems to be a preferred way.

Not sure what does toolchain looks like here, but it should still use the full path.
Problem with PATH is you're not suppose to expose this to whatever Bazel's calling.
They may not like to PATH required by PATH.
PATH is used by everything, but things like CC has a much narrower context.

The customized toolchain does need full paths. The auto generated toolchain is also using the full paths, but the default auto toolchain generator does not like full paths in CC for whatever reason.

In this particular case, we set the PATH variable only for bazel build command, and we know there are only our ccache wrappers in that added PATH.

scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
scripts/codebase_utils.sh Outdated Show resolved Hide resolved
scripts/toolchain.sh Outdated Show resolved Hide resolved
@chenhao94 chenhao94 merged commit fee58b9 into master Jun 11, 2022
@chenhao94 chenhao94 deleted the ch/new-toolchain branch June 11, 2022 18:54
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.

2 participants