Skip to content

Conversation

@h0tw1r3
Copy link

@h0tw1r3 h0tw1r3 commented Apr 22, 2024

hashFiles currently doesn't support globs outside of the GITHUB_WORKSPACE. This works around the limitation.

2bndy5
2bndy5 previously requested changes Apr 22, 2024
Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, this may not cache the clang-format nor clang-tidy executables.

Important

This might actually cache the executables on MacOS or Windows because our cpp-linter/clang-tools-pip tool will fall back to installing clang tools (static) binaries to the same directory that the python executable is located.
In the case of a venv, that default install location is /.env/Scripts/ on Windows or /.env/bin/ on MacOS (~/.local/bin/ on linux).

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 22, 2024

FYI, I think the CI failed on MacOS runners because of segfaults. This isn't anything to do with the changes here, rather we should look into this separately.

@2bndy5 2bndy5 dismissed their stale review April 22, 2024 20:29

resolved

@2bndy5 2bndy5 added enhancement New feature or request minor A minor version bump labels Apr 22, 2024
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 22, 2024

I also think there should be some logic added to detect presence of a cached venv before creating a venv.

${{ steps.setup-python.outputs.python-path }} -m venv "$GITHUB_ACTION_PATH/venv"

${{ steps.setup-python.outputs.python-path }} -m venv "$env:GITHUB_ACTION_PATH/venv"

The above lines might fail if the venv folder already exists.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 22, 2024

I guess we should also cache pip's global cache as well if this new option is enabled.

@h0tw1r3 h0tw1r3 marked this pull request as draft April 22, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor A minor version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants