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

CCACHE_DIR environment variable is ignored by action #96

Closed
n3world opened this issue Aug 23, 2022 · 8 comments · Fixed by #217
Closed

CCACHE_DIR environment variable is ignored by action #96

n3world opened this issue Aug 23, 2022 · 8 comments · Fixed by #217

Comments

@n3world
Copy link

n3world commented Aug 23, 2022

If the CCACHE_DIR environment variable is set ccache will use that for its cache location but ccache-action is hardcoded to use .${ccacheVariant}. The action should either honor CCACHE_DIR or set it to the hard coded path.

@jonashaag
Copy link
Contributor

I agree. Do you want to send a PR?

@jonashaag
Copy link
Contributor

Maybe we can even stop overriding the cache dir. I’m not sure why we do it.

@n3world
Copy link
Author

n3world commented Aug 24, 2022

Maybe we can even stop overriding the cache dir. I’m not sure why we do it.

Without any context my guess is this is done to keep all files generated during the action in the workspace directory. It might be good to keep this behavior and override the environment variable by using the --dir argument to specify the directory in all calls to ccache.

Even though the cache_dir is set in the config that is overriden by the environment variable. https://ccache.dev/manual/latest.html#_configuration

@hendrikmuhs
Copy link
Owner

@n3world Whats the use case of overriding CCACHE_DIR? Was it set accidentally?

@n3world
Copy link
Author

n3world commented Aug 29, 2022

@n3world Whats the use case of overriding CCACHE_DIR? Was it set accidentally?

I do not think I know exactly what you are asking so I'll just explain the situation a bit more.

CCACHE_DIR is currently set in the environment in which this action runs. This action does not check the setting of that variable and always tries to use <workdir>/.ccache. ccache prefers environment variables over settings in the configuration file so when the variable is set ccache does not use the directory this action expects and this action does not work.

Two ways to resolve this would be this action honors the environment variable and puts everything there or it overrides it and uses the directory it wants to. I was suggesting to go with the later fix since that provides more isolation and less likely to have issues if multiple compilations are using the same filesystem, fighting over setting configuration values in the configuration file and cache bloat.

If you were actually asking why the environment variable is set in the first place I don't think that it is too important but for me it is part of the docker container environment. The container is used for development and there it is convenient to have one volume location for the ccache just for consistency in scripts across all users.

@hendrikmuhs
Copy link
Owner

hendrikmuhs commented Aug 29, 2022

Thanks @n3world, that makes it more clear to me. You explained it is a side effect of re-using a docker container for development and CI. That's what I meant with "accidental". I understand you are not asking for implementing the possibility to override CCACHE_DIR, but to fix the unexpected behavior you see. So there isn't a use case, there is "just" developer pain, spending time debugging why the actions isn't working the way it should.

However this is harder to fix than it seems. Afaik steps in a workflow are isolated. Removing CCACHE_DIR from the environment won't work, because it is only unset in the setup action step, compilation runs in its own step and CCACHE_DIR might just be there again. I guess you introduce it in your build scripts. At the moment I can only think of checking if CCACHE_DIR is set and either log a warning or even fail the action. But if CCACHE_DIR is set in build scripts I wonder if we are even able to detect it at all. Again the action finalizer runs in its own step and likely doesn't have CCACHE_DIR set. With other words: I am not sure the action can honor CCACHE_DIR even if we want to implement support.

@n3world Can you tell me where CCACHE_DIR is set in your case?

I suggest to add a troubleshooting section in the Readme.

@n3world
Copy link
Author

n3world commented Aug 31, 2022

However this is harder to fix than it seems.

I know in steps you can write to $GITHUB_ENV file to set an environment variable that are seen by subsequent steps. The same could be done in the setup action

But if CCACHE_DIR is set in build scripts I wonder if we are even able to detect it at all. Again the action finalizer runs in its own step and likely doesn't have CCACHE_DIR set. With other words: I am not sure the action can honor CCACHE_DIR even if we want to implement support.

If it is just set in the build script and not global this action couldn't do anything about that, other than just documenting that CCACHE_DIR being set breaks this action

@n3world Can you tell me where CCACHE_DIR is set in your case?

It is in the environment for the docker image so it is there for anything using that image.

@krlmlr
Copy link
Contributor

krlmlr commented Oct 8, 2023

Getting back to the question raised in #96 (comment): why do we need to override the cache directory?

In particular, storing it inside the worktree causes unintended consequences for actions that push back to the Git repository. If we must override, I propose to store it alongside the worktree.

hendrikmuhs pushed a commit that referenced this issue Jul 23, 2024
Currently, if environment variables are set for CCACHE_DIR or SCCACHE_DIR then this action will ignore them and hardcode its own.

This is a problem if CCACHE_DIR is set for example in a docker file environment which is very hard to eliminate

Fixes #96
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 a pull request may close this issue.

4 participants