[Second review] SE-0386: `package` access modifier

At least half of these categories won’t be affected at all. For the rest, what would make their work easier, in the aggregate, is for the compiler not to presuppose any convention and instead let any such enforcement happen at another level.

yes. for example i can have a group of "MongoDB" modules that import a group of "BSON" modules because MongoDB uses BSON. but the MongoDB modules don't need access to certain APIs that are only used between the BSON modules themselves.

3 Likes
Ok, then I’ll make my next post take up the minimum amount of visual space, because to me it seems important, but it’s inextricably linked to doubts about the name:

I think this is the second or third time that I’ve seen @taylorswift mention this concept (the distinction, not the particular name choices), but I don’t have a clear sense of what the official response is. Does The Workgroup agree that the need to version and distribute code is different from the need to organize code at a large scale?

At a practical level, I’m firstly unhappy that I am going to be forced to deal with versioning if I want to take advantage of the organizational power of the new keyword.

I’m also unhappy that if my actual use case were to be eventually supported (by way of named “logical packages” within an SPM package) then I would permanently be dealing with the overloaded term “package” to refer to two distinct concepts, both of which I need to use.

If I understand correctly, I’m in a group that is considered to be a minority small enough that these problems are worth it for the greater good.

I think though that our minority may not actually be that small, e.g.:

Lastly, I think I might genuinely be in the minority on this final issue, I’m not sure…

Leaving aside the first two concerns of mine, there’s this:

I am curious to know why this is not a concern. This was the main issue I raised in the last thread, and now that we are already considering a future with “logical packages” nested within an SPM package (and possibly within each other?) to me the potential for exactly this confusion (or worse, closing off a desired syntax door for defining “logical packages”) is just exacerbated.

Bike shedding What about something like `in(package)` which could also then generalize to `in(file)`, `in(folder)`, etc. ?
4 Likes

I would caution against extrapolating this example, because calling that single monorepo "a single package" is just barely accurate today only because this proposal hasn't landed to provide the mechanism for the build system to pass different package identifiers to the compiler. I can state with some authority that if/when the package modifier lands, Bazel at least would use something more fine-grained than the entire repo (such as Bazel packages, which align very nicely with this) to define the package barrier for Swift compilation in that pipeline.

Likewise, I would be shocked if there are any monorepos of significant scale out there that are defined using a single top-level package manifest that describes every module underneath. I would expect that to be an extremely undesirable and unusable model (just as it would be similarly undesirable for a monorepo using Bazel to have a single top-level BUILD file), and I would expect heavy users of SPM instead to break their monorepo up into multiple package manifests that reference each other.

10 Likes

I'm glad to see this proposal moving forward and I appreciate the workgroup's detailed response to the first review.

+1. This proposal addresses a real use case we have in our Swift code-base at the company I work for.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes

  • Does this proposal fit well with the feel and direction of Swift?

Yes

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Followed the first review very closely.

1 Like

i think we are once again running aground the overloaded terminology surrounding “package”, because you are talking about the package identifier string passed to the swift compiler, and the swift compiler knows nothing about versioning.

but the SPM does know about versioning, and not only does it know about versioning, it aggressively pushes a particular 1-to-1 semver-based packaging and distribution paradigm that simply does not scale. it has nominal support for things like local filesystem dependencies, but these are relegated to the realm of “local testing and iteration” and utterly banned from participating in wider semver world.

using SPM’s definition of a “package”, our mutual friends really do develop one humongous giga-package, since all of that code shares a history. it doesn’t make sense to simultaneously depend on one subset of that code base at cl/XXX and another subset of that code base at cl/YYY - it’s all the same repo! but it does make sense for an SPM project to depend on two upstream “packages” without expecting any chronological relationship between those two “packages”. indeed, that is the entire function of semantic versioning.

you are entirely correct. i would go so far as to say the number of such monorepos is likely zero. this is one of the reasons i say SPM does not scale. SPM currently believes in one package with one manifest in one repository, and this cannot continue.

which brings me to this quote i recall from the previous review thread:

i think that when an SPM “project” reaches a certain size, it also becomes unavoidable that we need to stop thinking of ourselves as writing a “package”, we must start thinking of ourselves as writing many “packages” that share a repository and version series, and using the swift package manager to “manage” N > 1 packages. and it is unfortunate that SPM today does not truly support this level of sophistication.

and i think the reason SPM cannot support this kind of organization, is because of an awful decision we made several years back, to identify a package by the URL of its repository, and to use that repository to model a dependency on a package.

dependencies:
[
    .package(url: "https://github.com/SDGGiesbrecht/swift-markdown",
        .upToNextMinor(from: "0.50700.0")),
]
targets:
[
    .target(name: "MarkdownParser", dependencies:
        [
            .product(name: "Markdown", package: "swift-markdown"),
        ]),
]

and in this mental model, there is no room to decouple the repository from the package. from a target dependency perspective, the "swift-markdown" identifier defined by the package manifest’s name field represents the “package”. but from a dependency resolution perspective, the "https://github.com/SDGGiesbrecht/swift-markdown" repository URL represents the “package”.

which one is the real “package”? up until now, we did not care, because a package and a git repository were the same thing. but now we must care.

5 Likes

Should package as a language feature be reviewed independently of the build system? I'm not sure why this review is so tied to SwiftPM. We should be considering new language access control and keywords as a standalone review. Whatever build system used (SwiftPM, Bazel, etc) is an implementation detail to be reviewed afterwards.

Also disappointed that the review feedback here didn't mention the alternative of having all internal members accessible to package types. Additionally, the code churn that the past addition of fileprivate caused.

2 Likes

There’s a few more days in this review. I’d like to specifically invite feedback on the spelling of group: .excluded in SPM manifests.

I read it back when the second review first started and for a moment mistook it for an actual group (such that two excluded targets could access one another). But I had no good counter suggestions, so I did not post about it. group: nil is all I can think of, but that dares you to mistake it for the default.

Since you're asking, I don't think group: is a great spelling for the package manifest. There's actually no concept called "group" in this proposal and it's unclear if this direction will be pursued. This is about controlling the package access modifier, so I'd call it packageAccess: .excluded. I think it's nice to use the word "package" here to show the link with the package keyword.

If we later want to have groups, then it could be packageAccess: .group("MyGroup").

15 Likes

I would concur that I struggle a bit with reconciling the proposed revision with the overall direction that's been approved:

If our conclusion is that we are "loathe to pick another name that would be nearly synonymous with 'package' in the vast majority of cases," we will have sort of immediately turned around and done what we are loathe to do by the invention of an inner subdivision of the package known as a group and turning "package" into an elision for "the default package group."

Something like the suggestion above of explicitly designating packageAccess (we can workshop the name if this direction is desired) and then introducing "grouped" variations at a later time might seem like only a slight twist but to my mind would leave the overall vision much more intact.

7 Likes

Definitely agree.

Maybe then even something like:

packageAccess: .currentTarget

(Instead of just excluded, try to be more specific)

i am comfortable (and increasingly leaning towards) redefining “package” to mean group (hence renaming this field package), and using a more correct term, like “repository”, to refer to what we today refer to as an SPM “package”.

instead of depending on “packages”, we could depend on repositories, which could vend multiple packages (“groups”).

dependencies:
[
    .repository(url: "https://github.com/SDGGiesbrecht/swift-markdown",
        .upToNextMinor(from: "0.50700.0")),
]

i continue to insist that “groups” must never have version requirements, if this means we need to give up on the idea of “package versions” in favor of “repository versions”, so be it.


aside: i really do not like the word “group”, as proposed this is essentially what bazel, etc. calls a “package”, which implies that an SPM repository is a group of packages… which also contains… “package groups”?

5 Likes

The "group" discussion brings back the question to me: What’s the real advantage of package spelling over something like @group(PackageName) (indicating internal), with SwiftPM gaining fine-grained control on whether a specific SPI can be accessed from a target?

We don’t typically use SPI with the internal access level, but this does give us some room to rethink what it could be. As the name suggests, this is internal, and should not be considered as part of the public API. Meanwhile, it can be shared by all targets in group PackageName, as some shared implementation detail. SwiftPM could have the ability to hide such SPI from dependent modules. It also has the ability to enable the SPI by default for targets in the group.

based on this passage:

my interpretation is the language committee believes @spi and things resembling it are a different feature from package. this implies that @spi-shaped things like @group(PackageName) are out of scope for this proposal.

in my opinion, SPM gaining fine-grained control over SPIs is less valuable (and probably more difficult to implement) than SPM gaining the ability to delimit “packages” in a more sophisticated way than “everything in this whole giant repository minus excluded targets”. it saddens me that we keep reaching for “Create New Repository” just because our build system guides us towards that sort of organization.

3 Likes
One extra point challenging the assumption that "multiple nuclei" is uncommon:

Even if it could be generally agreed upon that it is uncommon to have "multiple nuclei" as an end goal for one's repository architecture, I think it will always be common that code that eventually becomes a reusable tool that is accessible at its own repository is initially conceived and fleshed out inside of some other higher level package that the tool was initially conceived to serve.

If the "multiple nuclei" use case were supported I believe that it would instantly become my preferred method of simultaneously iterating on a higher level package and a lower level supporting tool, because I get to skip the step of creating a repository and starting up a version history. I think that reducing the friction involved in starting the process of factoring out a new "nucleus" ("package") could cause more people to take the step. Eventually, when and only when the nucleus is ready (which I think is one of @taylorswift 's most emphasized points), it can be promoted to having its own repository and being shared across many projects. The fact that we would be making it easier for lower level tools to incubate in close contact with their first real client before committing to being published could result in a generally higher quality of packages in our ecosystem.

3 Likes

Ellie has revised the proposal to replace group: .excluded with packageAccess: false. That idea wasn't discussed here as a concrete plan, so the review period has been extended until next Monday, May 1st to give folks an opportunity to weigh in on that issue specifically.

John McCall
Review Manager

9 Likes

I like this revision: it hews to the reasoning laid out for accepting the rest of proposal, and the addition serves its purpose just so without extra ceremony.

3 Likes

I really like this idea, even better than having a group: parameter in the manifest, because it also tackles the issue of extremely large manifests that would benefit from splitting even just for readability and maintainability reasons, regardless of the package access.

3 Likes

SE-0386 has been accepted.

Thank you to everyone who participated in this review.

John McCall
Review Manager

2 Likes