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.

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

feat: Swift registry support #1043

Closed
wants to merge 9 commits into from
Closed

Conversation

watt
Copy link
Contributor

@watt watt commented Apr 25, 2024

Fixes #1016

This PR adds support for package registry dependencies, by adding a new build rule type. For a Package.swift dependency like this:

    dependencies: [
        .package(id: "apple.swift-collections", from: "1.1.0"),
    ]

A build rule will be generated like this:

    swift_registry_package(
        name = "swiftpkg_apple.swift_collections",
        dependencies_index = "@//:swift_packages_index.json",
        id = "apple.swift-collections",
        version = "1.1.0",
    )

@watt watt mentioned this pull request Apr 25, 2024
@luispadron luispadron self-requested a review April 25, 2024 03:27
Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

This is a great start thank you!

examples/registry_example/README.md Outdated Show resolved Hide resolved
examples/registry_example/swift_deps.bzl Outdated Show resolved Hide resolved
gazelle/internal/spdump/dependency.go Show resolved Hide resolved
gazelle/internal/spreso/pin_store_v2.go Outdated Show resolved Hide resolved
gazelle/internal/spreso/pin_store_v2.go Outdated Show resolved Hide resolved
gazelle/internal/swift/code_dir.go Show resolved Hide resolved
gazelle/internal/swift/repo_rule_from_bazel_repo.go Outdated Show resolved Hide resolved
Comment on lines +35 to +36
# requires bazel 7.1
headers = headers,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably fine for a new feature, we should call this out in any documentation though. I wonder if folks on Bazel < 6 but > 5 could use the credentials helper to work around it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably fine for a new feature, we should call this out in any documentation though. I wonder if folks on Bazel < 6 but > 5 could use the credentials helper to work around it?

I'm not familiar with the credentials helper; can it override the Accept header?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like requiring Bazel 7.1. I'm also not familiar with the credentials helper. @luispadron do you know an answer to their question?

Copy link
Collaborator

@luispadron luispadron May 29, 2024

Choose a reason for hiding this comment

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

I experimented with it to get private GitHub repos working and works well for that but looks like things like Accept aren't supported, least going off of: bazelbuild/bazel#17829 (comment)

swiftpkg/internal/swift_registry_package.bzl Outdated Show resolved Hide resolved
@cgrindel
Copy link
Owner

Thank you for the contribution! Let me know what you think about my comment on the issue.

gazelle/internal/spreso/pin_store_v2.go Outdated Show resolved Hide resolved
swiftpkg/internal/swift_registry_package.bzl Outdated Show resolved Hide resolved

def _read(repository_ctx):
registry_config_json = repository_ctx.read(
Label("@//:.swiftpm/configuration/registries.json"),
Copy link
Owner

Choose a reason for hiding this comment

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

We can add an attribute defined as attr.label(allow_single_file = True) to swift_registry_package(). We can read the value in _swift_registry_package_impl() and pass the path to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I created a registry_config attribute for this. It still defaults to Label("@//:.swiftpm/configuration/registries.json"), rather than emitting that to the generated rules, though. Is that OK? I'm worried that letting this be customized is a bit dangerous, because putting any other value in here means it won't be using the same config that swift package resolve does.

I think we might want to also pass a --config-path arg to swift to prevent it from using global config outside the workspace.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the delay. I had many family obligations over the past 2 weeks.

Regarding the label default, that is fine. In another comment, I suggest that we update the doc for the attribute stating where the file comes from / how it is typically generated.

I think we might want to also pass a --config-path arg to swift to prevent it from using global config outside the workspace.

Where do you want to add that?

Copy link
Contributor Author

@watt watt May 28, 2024

Choose a reason for hiding this comment

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

I think we might want to also pass a --config-path arg to swift to prevent it from using global config outside the workspace.

Where do you want to add that?

In the SwiftBin wrapper that calls swift package resolve. By passing a --config-path to that command, we can make swift only look at the config in this workspace (which is the only one RSPM is looking at). So the command would become something like:

swift package resolve --config-path .swiftpm/configuration

--config-path sets the location of the shared config, which is usually ~/.swiftpm/configuration. The idea here is just to make it identical to the project config so that there's only one config to consider. Setting it to something like /dev/null would probably have the same effect (but is maybe more fragile if a new version of swift decides the directory must exist).

Copy link
Owner

Choose a reason for hiding this comment

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

OK. I am fine with having the new parameter passing the --config-path value. However, as I may have mentioned in another thread, the swift_update_packages macro may disappear before 1.0. I will open a discussion about that shortly.

@watt watt changed the title [WIP] Swift registry support feat: Swift registry support May 15, 2024
@watt watt marked this pull request as ready for review May 15, 2024 23:07
@brentleyjones
Copy link
Collaborator

@watt @cgrindel What's needed to land this? Ideally this would be part of 1.0.

@cgrindel
Copy link
Owner

What's needed to land this? Ideally this would be part of 1.0.

We just need to understand how this PR changes with swift_update_packages disappearing from rules_swift_package_manager.

@watt
Copy link
Contributor Author

watt commented Jun 25, 2024

What's needed to land this? Ideally this would be part of 1.0.

We just need to understand how this PR changes with swift_update_packages disappearing from rules_swift_package_manager.

Is this something I can help with? I'd love to land this as it works today and then iterate towards whatever shape 1.0 has, but I don't have a good sense for what that entails.

@cgrindel
Copy link
Owner

@watt I just merged #957 which removes the swift_update_packages and some other big changes. Can you rebase this PR from main and raise any issues that you may need help navigating?

@watt
Copy link
Contributor Author

watt commented Aug 27, 2024

Unfortunately the breaking changes in #957 made it impractical for me to continue working on this. I may revisit it at some point once we have migrated to bzlmod.

@watt watt closed this Aug 27, 2024
@cgrindel
Copy link
Owner

I am sorry that the recent changes thwarted your efforts. Once the 1.0 release happens, it would be great to pick this up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swift registry support
4 participants