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

Force TOOLCACHE_ROOT to be equal AGENT_TOOLSDIRECTORY #338

Merged
merged 4 commits into from Apr 15, 2022
Merged

Force TOOLCACHE_ROOT to be equal AGENT_TOOLSDIRECTORY #338

merged 4 commits into from Apr 15, 2022

Conversation

dsame
Copy link
Contributor

@dsame dsame commented Feb 17, 2022

Description:
The AGENT_TOOLSDIRECTORY variable tells where python has to be installed to
The RUNNER_TOOL_CACHE variable tells where python expected to be installed in

If these 2 variables are different the actions/tool-cache.find does not find a python instance installed by actions/setup-pyton.installCpythonFromRelease

Test build for self-hosted agent with AGENT_TOOLSDIRECTORY set(the original issue condition): https://github.com/akv-demo/setup-python-test/runs/5237321231?check_suite_focus=true

Test for builds for hosted agents (with AGENT_TOOLSDIRECTORY set and omitted)

https://github.com/akv-demo/setup-python-test/runs/5237321104?check_suite_focus=true
https://github.com/akv-demo/setup-python-test/runs/5237321426?check_suite_focus=true

Upd: last changes sample builds https://github.com/akv-demo/setup-python-test/actions/runs/1874490317

Note:
It seems we should not apply this fix but remove the mention of AGENT_TOOLSDIRECTORY from the documentation. In this case the python will be installed into _work/_tool/Python/3.7.7/x64 directory

see https://github.com/akv-demo/setup-python-test/runs/5237321176?check_suite_focus=true

Related issue:
Add link to the related issue.

Check list:

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

@dsame dsame marked this pull request as ready for review Feb 17, 2022
src/setup-python.ts Outdated Show resolved Hide resolved
src/setup-python.ts Outdated Show resolved Hide resolved
@nialloh23
Copy link

@nialloh23 nialloh23 commented Mar 3, 2022

Just wondering if there is an idea of when this PR will be merged? And if it fixes the issues seen setting up pyhon versions on self hosted runners? Thanks a mill

@Nick-Yazdani
Copy link

@Nick-Yazdani Nick-Yazdani commented Mar 30, 2022

Just wondering if there is an idea of when this PR will be merged? And if it fixes the issues seen setting up pyhon versions on self hosted runners? Thanks a mill

Yeah having this same issue and it would be great to get this PR merged

@dsame dsame requested a review from as a code owner Apr 6, 2022
Copy link
Contributor

@brcrista brcrista left a comment

Ooooh ... this is confusing. Here's how these variables work:

It seems we should not apply this fix but remove the mention of AGENT_TOOLSDIRECTORY from the documentation. In this case the python will be installed into _work/_tool/Python/3.7.7/x64 directory

I think you're right about this. Some relevant discussion from when the docs were written: #90 (comment)

More specifically, I think the right fix is to replace AGENT_TOOLSDIRECTORY with RUNNER_TOOL_CACHE in the README. Could you try testing that and see if it works?

cc @konradpabjan since you might have some additional context

@dsame
Copy link
Contributor Author

@dsame dsame commented Apr 7, 2022

@brcrista
Hello Brian, it seems i got the idea why the variable AGENT_TOOLSDIRECTORY was introduced and we need to merge this PR.

This variable is useful if an user wants to install the python to the other location than the default _work/_tool/Python/3.7.7/x64

I tried to set RUNNER_TOOL_CACHE instead of AGENT_TOOLSDIRECTORY as you advised but it is ignored and the python is installed in its default location _work/_tool/Python/3.7.7/x64

Summary: i am 90% sure we need both apply the fix of this PR and make a change in the docs to make clear AGENT_TOOLSDIRECTORY is an optional variable and should be used only if an user wants to change the default location

NOTE: i can do more investigations of why RUNNER_TOOL_CACHE was ignored in my build, does it make sense?

Copy link
Contributor

@brcrista brcrista left a comment

Hm, ok, I guess there's something I'm missing then. Let's go ahead and merge this.

Using the legacy variable isn't ideal, but the most important thing is to have it documented correctly. So if you could just follow up with a change to the docs, that would be great.

@dsame
Copy link
Contributor Author

@dsame dsame commented Apr 8, 2022

Hm, ok, I guess there's something I'm missing then. Let's go ahead and merge this.

Using the legacy variable isn't ideal, but the most important thing is to have it documented correctly. So if you could just follow up with a change to the docs, that would be great.

And the last note - i've checked the existing README and the wording "The Python packages that are downloaded from actions/python-versions are originally compiled from source in /opt/hostedtoolcache/ ... which makes them non-relocatable." explains the AGENT_TOOLSDIRECTORY is not optional and nothing in README should be changed. So we end up with merging this PR.

@brcrista
Copy link
Contributor

@brcrista brcrista commented Apr 8, 2022

In that case, I wonder if it would just make sense to force AGENT_TOOLSDIRECTORY=/opt/hostedtoolcache/ in this action?

Anyway @dsame @dmitry-shibanov feel free to merge this PR when you're ready.

@dsame dsame mentioned this pull request Apr 15, 2022
5 tasks
@dsame
Copy link
Contributor Author

@dsame dsame commented Apr 15, 2022

In that case, I wonder if it would just make sense to force AGENT_TOOLSDIRECTORY=/opt/hostedtoolcache/ in this action?

Anyway @dsame @dmitry-shibanov feel free to merge this PR when you're ready.

helllo @brcrista ,

I agree the current configuration is overcomplicated, but i asked to @marko-zivic-93 to merge this PR asap because we have many related issue.

I'll create the another PR soon to eliminate the need of the set the AGENT_TOOLSDIRECTORY for self-hosted agents

@marko-zivic-93 marko-zivic-93 merged commit 91712e1 into actions:main Apr 15, 2022
96 checks passed
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

6 participants