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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Tool Path handling for self-hosted runners #466

Merged
merged 6 commits into from Jul 26, 2022

Conversation

techman83
Copy link
Contributor

@techman83 techman83 commented Jul 18, 2022

Description:
This removes the manual paths that were set to address issues with self-hosted runners. However upon further investigation my previous attempt wasn't quite correct. It did restore the out of the box functionality for self-hosted runners, but it didn't address what #394 was intending to fix.

An improvement could be to handle AGENT_TOOLSDIRECTORY further in the code, as it seems a bit incorrect to override the RUNNER_TOOL_CACHE, however this seems to address all use cases and is an improvement 馃檪

Related issue:
#459
Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@techman83 techman83 requested a review from a team as a code owner Jul 18, 2022
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@IvanZosimov
Copy link
Contributor

IvanZosimov commented Jul 20, 2022

Hi, @techman83 馃憢 Unfortunately, Python binaries for macOS are indeed non-relocatable (you can check it by yourself using a self-hosted runner with macOS and setup-python v4.0.0 without setting AGENT_TOOLSDIRECTORY) and there is no simple way to make them relocatable, so may I ask you to get back to previous logic of assigning AGENT_TOOLSDIRECTORY but only for macOS ( '/Users/runner/hostedtoolcache' ) ? In that case, we also have to revert some changes in README.md namely related to creating '/Users/runner/hostedtoolcache' on macOS and granting permissions.

@techman83
Copy link
Contributor Author

techman83 commented Jul 20, 2022

@IvanZosimov that's unfortunate, I don't have access to a Mac, but I will expand the PR to cover that case. I'll add some clear documentation to go with it.

@techman83
Copy link
Contributor Author

techman83 commented Jul 21, 2022

@IvanZosimov I have a way to validate the issue, and will have to document / ensure we complain loudly that AGENT_TOOLSDIRECTORY is not valid for mac, self hosted or otherwise. Though I may submit a PR upstream, going down the rabbit hole to understand why, I can see it.

@techman83
Copy link
Contributor Author

techman83 commented Jul 22, 2022

I have some small tweaks to the commits messages to do, and the docs to add. But that should be a clean and clear message in the process. Feedback welcome!

src/setup-python.ts Outdated Show resolved Hide resolved
@techman83
Copy link
Contributor Author

techman83 commented Jul 23, 2022

Alright, I tweaked it slightly as AGENT_TOOLSDIRECTORY isn't applicable for Mac due to the non-relocatable flag. It's probably worth raising a PR upstream, as it looks like a relatively minor change (at least at a glance). Plus fixed up the commits + conflicts.

@techman83
Copy link
Contributor Author

techman83 commented Jul 25, 2022

Noticed the prettier was upset, so ran that through and fixed it up.

@IvanZosimov
Copy link
Contributor

IvanZosimov commented Jul 26, 2022

Hi, @techman83 馃憢 Thanks for the changes, we are going to merge this PR soon, could you, please, sync it with the main branch to resolve conflicts?

techman83 added 5 commits Jul 26, 2022
This updates and simplies the tool cache documentation to match the implementation in
both  and

  Relates actions#459
This fixes the tool cache path for self-hosted runners, along
with handling AGENT_TOOLSDIRECTORY for both hosted + self-hosted.

    Fixes actions#459
Shared libraries for the Mac python builds are not configured with the
relocatable flag, thus must always be configured with the hosted path.

Relates actions#459
Ensure that the path requirements and reasoning is clear, to reduce
confusion when using self-hosted, or attempting to set an
'AGENT_TOOLSDIRECTORY' environment variable.
@techman83
Copy link
Contributor Author

techman83 commented Jul 26, 2022

Hi @IvanZosimov, all done!

src/setup-python.ts Outdated Show resolved Hide resolved
@marko-zivic-93 marko-zivic-93 merged commit a93d541 into actions:main Jul 26, 2022
163 checks passed
@techman83 techman83 deleted the fix/tool_path branch Jul 27, 2022
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

5 participants