Merge tools now that OS-specific keys are possible - #1768
Conversation
Rename the `uses` tool property to `deps` in `emsdk.py` and `emsdk_manifest.json`. See #1768 for an example of when this name makes more sense. We also use the term "deps" in emscripten already so this is more consistent. Update `--uses` flag description and parsing in `emsdk list` to use `--deps`.
57401a3 to
cbceb4c
Compare
| return self.url is not None | ||
| # Tools/SDKs without an explicit 'os' filter are compatible if they specify | ||
| # a download URL *or* have dependencies. | ||
| return self.url is not None or bool(self.deps) |
There was a problem hiding this comment.
The reason this is needed is the previously the "sdks" were listed as one-per-os, but now there is just a single one with deps and deps_linux.
Prior to this change the return statement above on line 1963 would always fire for SDKs
|
|
||
| return self.url is not None | ||
| # Tools/SDKs without an explicit 'os' filter are compatible if they specify | ||
| # a download URL *or* have dependencies. |
There was a problem hiding this comment.
But why does having dependencies make a tool compatible? E.g. some build system might only run on linux/mac, so it has deps for each of those, but windows wouldn't want it?
That is, having deps is a "structural" property. it doesn't say who or what should use it.
There was a problem hiding this comment.
If if you only specify mac_deps and windows_deps and then on linux the self.deps would be empty here. i.e. it would be uninstallable.
What this logic is saying is that to be installable you need to either have a url or deps for the current platforms. If you have neither of them then you are not installable.
i.e. its ok to have url for current platform or not deps for the current platform, but its not OK to have neither.
The reason this wasn't needed because is that the SDK had an entry for each OS with a specific os = 'linux'. Now that the SDK is generic we need this line to be able to deal with a tools with no URL and only deps (i.e. an SDK).
Yes another way of putting it: Prior to this change all the deps-only tools were OS-specific, but there is no reason you can't have a deps-only tool that is generic, and that requires this change (or something like it).
There was a problem hiding this comment.
I update the comment to make this more clear. Both self.url and self.deps are by construction working on the current OS.
Since #1761 we now have os-specific keys so we don't need separate entries in these cases anymore.
| return self.url is not None | ||
| # Tools without an explicit 'os' are compatible with the current | ||
| # OS if they have URL for the current OS *or* a set of dependencies | ||
| # for the current OS. |
There was a problem hiding this comment.
| # for the current OS. | |
| # for the current OS (`deps` by itself, unlike `deps_linux`, applies to all OSes, so all are compatible with it). |
There was a problem hiding this comment.
Its a bit more subtle than that. self.deps is the deps for the current OS. So if either deps_linux or deps (no suffix) exist in the file then deps will be non-null here.
2 participants
Since #1761 we now have os-specific keys so we don't need separate entries in these cases anymore.