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.

Sign up for GitHub

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

Jump to bottom

Add activated_cfg for ccache #1345

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add activated_cfg for ccache #1345

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions emsdk_manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,7 @@
"bitness": 64,
"url": "https://github.com/juj/ccache.git",
"git_branch": "emscripten",
"activated_cfg": "CCACHE=%installation_dir%/bin",
Copy link
Collaborator

Choose a reason for hiding this comment

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

CCACHE is not a recognized setting the emscripten config file as far as I can tell.

The valid config settings are listed here: https://github.com/emscripten-core/emscripten/blob/7cdfa8d9f22562e7c08789a5ae29ad9a1025a8e4/tools/config.py#L122-L140 (not the CACHE is unrelated to ccache).

Copy link
Author

Choose a reason for hiding this comment

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

Ah, should I add a CCACHE setting there then? FWIW, when I made this change locally, it worked fine without adding it to the config.py list.

"activated_path": "%installation_dir%/bin",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should add ccache to your PATH. You would need to be sure to include it in your emsdk activate command line though I think.

Copy link
Author

Choose a reason for hiding this comment

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

Without activated_cfg, this clause causes ccache to not be added to the path:

emsdk/emsdk.py

Lines 1733 to 1735 in c18280c

activated_cfg = self.activated_config()
if not activated_cfg:
return len(deps) > 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

activated_cfg doesn't have anything to do with the PATH. activated_cfg is used to add things to the emscripten config file. As far as I know ccache has not place in the emscripten config file.

The things that adds to your PATH is activated_path.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, then it would seem we need to change L1733-L1735 here, as this results in any tool without an activated_cfg and no deps being excluded. ccache is the only tool without an activated_cfg as far as I can tell.

Copy link
Author

Choose a reason for hiding this comment

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

I think if we change L1735 to return True then this will result in ccache always being active if it's installed? I'm not super familiar with how emsdk works—where is the list of currently activated tools stored?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it looks like we can completely remove those lines I think.

I'm not sure why would declare a tool inactive simply because it has no dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@juj any idea why these lines exist?

"activated_env": "_EMCC_CCACHE=1;CCACHE_CONFIGPATH=%installation_dir%/emcc_ccache.conf",
"cmake_build_type": "Release",
Expand Down