Add bazel_dep.max_compatibility_level #17572
Conversation
|
@Wyverald @meteorcloudy I split this into the refactor and the addition of the new property (maybe it can be imported that way?). I'm sure it needs more tests, and some cleanup, but I wanted to start to get feedback now. Thanks! |
brentleyjones
force-pushed
the
bj/add-bazel_dep.max_compatibility_level
branch
4 times, most recently
from
bb2cae3
to
26c218a
Compare
brentleyjones
requested review from
Wyverald and
meteorcloudy
as code owners
brentleyjones
force-pushed
the
bj/add-bazel_dep.max_compatibility_level
branch
from
26c218a
to
7aef05f
Compare
sgowroji
added
team-ExternalDeps
| /** The version of the module. Must be empty iff the module has a {@link NonRegistryOverride}. */ | ||
| public abstract Version getVersion(); | ||
|
|
||
| /** The maximum compatibility level of the module. */ |
There was a problem hiding this comment.
must be 0 iff the module has a non-registry override?
also, we should mention that "0" has a special meaning -- no maximum enforced
| @@ -78,7 +78,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) | |||
| if (root == null) { | |||
| return null; | |||
| } | |||
| ImmutableMap<ModuleKey, Module> initialDepGraph = Discovery.run(env, root); | |||
| ImmutableMap<UnresolvedModuleKey, UnresolvedModule> initialDepGraph = Discovery.run(env, root); | |||
There was a problem hiding this comment.
it's unclear to me why this initial dep graph is keyed by UnresolvedModuleKey, instead of ModuleKey. Is there a case where <foo, 1.2, 1> and <foo, 1.2, 2> correspond to different values in this map? From reading Discovery.java, it doesn't seem to be the case -- they both point to the result of ModuleFileFunction[<foo,1.2>].
So I suggest that we change "initialDepGraph" across the board to be keyed by ModuleKey, and rename "UnresolvedModuleKey" to something like "DepSpec" to clarify the fact that it's only ever used as the dependency specification in UnresolvedModule. (In fact UnresolvedModule also sounds a bit awkward to me. "Pre-selection module" would be better IMO; I'll keep thinking about this)
There was a problem hiding this comment.
Can you look at the tests first and see if your thoughts still hold? I had this as ModuleKey first, but (at the time at least) it made harder/impossible to do the selection logic. If this did change, then in selection we would have to create the same mapping anyway, in a much more inefficient way (by iterating through all of the dependencies of each module and building up UnresolvedModuleKey).
I also don't see an issue with this being keyed by UnresolvedModuleKey. I think it's fine that before selection we are dealing with unresolved module keys. After selection everything is ModuleKey and Module.
There was a problem hiding this comment.
Can you look at the tests first and see if your thoughts still hold?
I had a brief look, and I don't think they change my opinion. Selection is basically "unpruned dep graph (ModuleKey->UnresolvedModule) -> resolved dep graph (ModuleKey->Module)". Just judging from this interface, there's nothing that requries a map whose key is UnresolvedModuleKey. If you need to build a UnresolvedModuleKey (really "DepSpec" is a better way to think about it) to ModuleKey mapping during Selection, then by all means do that; we shouldn't pollute the interface types with implementation needs.
I think it's fine that before selection we are dealing with unresolved module keys.
My problem with that is it's conceptually wrong. The only thing that adding "max_compatibility_level" changes is that modules can ask for a special kind of "dep". It doesn't change the type of entities in the dependency graph, pre- or post-selection; it just means that we do some extra logic during selection. It's a tell that <foo, 1.2, 1> and <foo, 1.2, 2> will always point to the same object.
There was a problem hiding this comment.
I'll wait for all comments before making this change, but 👍.
brentleyjones
force-pushed
the
bj/add-bazel_dep.max_compatibility_level
branch
from
7aef05f
to
d09daf0
Compare
|
Friendly ping 🙇 |
Resolves #17378.