Skip to content

Conversation

@MaxDesiatov
Copy link
Contributor

We have to operate on two different graphs in the PackageGraph module:

  1. A graph of packages in PubGrub package resolution code;
  2. A graph of resolved products and modules that belong to those products.

Currently the second graph is misleadingly called PackageGraph, although that name is much more suitable for the first graph. This naming is confusing and unfortunate, and can be especially misleading for first-time contributors. We should better document the SwiftPM resolution and build pipeline in the future, but cleaning up naming is the first step.

I'm keeping old names as deprecated overloads or typealiases to make migration easier for SwiftPM clients. This renaming has no impact on any public stable modules like PackageDescription.

In the short term we should also split the PackageGraph module into PubGrub and ModulesGraph modules, but for now I'm renaming a single type to keep the PR manageable.

We have to operate on two different graphs in the `PackageGraph` module:
1. A graph of packages in PubGrub package resolution code;
2. A graph of resolved products and modules that belong to those products.

Currently the second graph is misleadingly called `PackageGraph`, although that name is much more suitable for the first graph. This naming is confusing and unfortunate, and can be especially misleading for first-time contributors. We should better document the SwiftPM resolution and build pipeline in the future, but cleaning up naming is the first step.

In the short term we should also split the `PackageGraph` module into `PubGrub` and `ModulesGraph` modules, but for now I'm renaming a single type to keep the PR manageable.
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) February 23, 2024 16:43
@MaxDesiatov MaxDesiatov self-assigned this Feb 23, 2024
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

In the short term we should also split the PackageGraph module into PubGrub and ModulesGraph modules, but for now I'm renaming a single type to keep the PR manageable.

Is PubGrub an implementation detail, or will it always be fundamental to the package resolution? Another name for the module would just be PackageResolution 😅.

ModulesGraph... Is that more of a TargetGraph? I notice our docs talk about "module" so maybe that does make sense, I just find it a little odd for eg. executables.

@MaxDesiatov MaxDesiatov disabled auto-merge February 23, 2024 16:49
@MaxDesiatov
Copy link
Contributor Author

Is PubGrub an implementation detail, or will it always be fundamental to the package resolution? Another name for the module would just be PackageResolution 😅.

I would prefer to keep PubGrub implementation details private, hence PackageResolution makes sense to me. Thanks for the suggestion!

ModulesGraph... Is that more of a TargetGraph? I notice our docs talk about "module" so maybe that does make sense, I just find it a little odd for eg. executables.

The Target naming is unfortunate, as it has at least three meanings in the code base:

  1. Target as in target/host triples
  2. Target as a target that's essentially a Swift or a Clang module.
  3. Build system target, which a node in a build graph, usually a single target per compilation unit.

In a follow-up PR I'd like to rename ResolvedTarget to ResolvedModule and so forth, up to PackageDescription, naming in which we have to preserve for compatibility reason. This would allow us to remove at least point 2 in that list of ambiguities, and we can keep uses of 1 and 3 mostly segregated in the codebase.

Another argument for "modules" instead of "targets" in this context is that we already have "module aliases" and not "target aliases", even though module aliases are attached per target in Package.swift. Ideally in the long term, I would love to see point 2 to disappear globally eventually. Even Xcode's use of targets doesn't match the current SwiftPM naming: Xcode targets correspond to SwiftPM products.

@MaxDesiatov MaxDesiatov requested a review from bnbarham February 23, 2024 17:15
@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Feb 23, 2024

I notice our docs talk about "module" so maybe that does make sense, I just find it a little odd for eg. executables.

For this one specifically: every executable still has an underlying module, IIUC we even generate a Clang module for C/C++ targets, so IMO this naming is general enough to be applicable to executables too.

@bnbarham
Copy link
Contributor

For this one specifically: every executable still has an underlying module

Yeah but that's where it doesn't really fit - we have an underlying module, but the executable is still a "module" in this scheme. I don't really have a better suggestion TBH and I understand the motivation for switching from target, so I'm not going to block it. Just thought it was worth mentioning.

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) February 23, 2024 17:52
@MaxDesiatov MaxDesiatov merged commit 6ff5cbd into main Feb 23, 2024
@MaxDesiatov MaxDesiatov deleted the maxd/modules-graph branch February 23, 2024 18:57
furby-tm pushed a commit to wabiverse/swift-package-manager that referenced this pull request May 15, 2024
We have to operate on two different graphs in the `PackageGraph` module:
1. A graph of packages in PubGrub package resolution code;
2. A graph of resolved products and modules that belong to those
products.

Currently the second graph is misleadingly called `PackageGraph`,
although that name is much more suitable for the first graph. This
naming is confusing and unfortunate, and can be especially misleading
for first-time contributors. We should better document the SwiftPM
resolution and build pipeline in the future, but cleaning up naming is
the first step.

I'm keeping old names as deprecated overloads or typealiases to make
migration easier for SwiftPM clients. This renaming has no impact on any
public stable modules like `PackageDescription`.

In the short term we should also split the `PackageGraph` module into
`PubGrub` and `ModulesGraph` modules, but for now I'm renaming a single
type to keep the PR manageable.
furby-tm pushed a commit to wabiverse/swift-package-manager that referenced this pull request May 15, 2024
We have to operate on two different graphs in the `PackageGraph` module:
1. A graph of packages in PubGrub package resolution code;
2. A graph of resolved products and modules that belong to those
products.

Currently the second graph is misleadingly called `PackageGraph`,
although that name is much more suitable for the first graph. This
naming is confusing and unfortunate, and can be especially misleading
for first-time contributors. We should better document the SwiftPM
resolution and build pipeline in the future, but cleaning up naming is
the first step.

I'm keeping old names as deprecated overloads or typealiases to make
migration easier for SwiftPM clients. This renaming has no impact on any
public stable modules like `PackageDescription`.

In the short term we should also split the `PackageGraph` module into
`PubGrub` and `ModulesGraph` modules, but for now I'm renaming a single
type to keep the PR manageable.
furby-tm pushed a commit to wabiverse/swift-package-manager that referenced this pull request May 15, 2024
We have to operate on two different graphs in the `PackageGraph` module:
1. A graph of packages in PubGrub package resolution code;
2. A graph of resolved products and modules that belong to those
products.

Currently the second graph is misleadingly called `PackageGraph`,
although that name is much more suitable for the first graph. This
naming is confusing and unfortunate, and can be especially misleading
for first-time contributors. We should better document the SwiftPM
resolution and build pipeline in the future, but cleaning up naming is
the first step.

I'm keeping old names as deprecated overloads or typealiases to make
migration easier for SwiftPM clients. This renaming has no impact on any
public stable modules like `PackageDescription`.

In the short term we should also split the `PackageGraph` module into
`PubGrub` and `ModulesGraph` modules, but for now I'm renaming a single
type to keep the PR manageable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants