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: SPM(Swift Package Manager) backend #2241

Merged
merged 17 commits into from
Jun 3, 2024
Merged

feat: SPM(Swift Package Manager) backend #2241

merged 17 commits into from
Jun 3, 2024

Conversation

kattouf
Copy link
Contributor

@kattouf kattouf commented Jun 2, 2024

Implementation of backend for install executables managed by Swift Package Manager

Description

Discussion
Compared to previous PR there are no dependencies on third parties other than Swift itself.

SPM does not support installing an executable, only the build command. This backend implementation itself covers this functionality and is described by a flow:

  1. Clone the repository with the SPM package into a temporary directory.
  2. Read Package.swift for a list of executables
  3. Build each executable and copy the binary with associated resources to mise install directory

Demo screenshots:
Screenshot 2024-06-02 at 14 14 53

Screenshot 2024-06-02 at 14 17 09

P.S. I'm not full-time Rust developer, feel free to make comments if something is not done according to best practices in Rust community

Requires attention

  1. Missed swiftdependency
    I had some problems installing swift with the current asdf plugin so I guess we'll just have to wait for Add Swift plugin #1708
    Now missed dependency leads to:
    • Unable to run e2e test
    • Users are responsible for installing swift
  2. Support for build artifacts on other platforms
    I have no experience using swift on other platforms (Linux, Windows) and can't be sure about finite list of resource and dynamic libraries formats we should support to copy together with binary.
    I decide dive into it after we are sure that everyone is happy with the feature and we will discuss merging it.
  3. Name of package to install
    1. I don't sure about how flexible it should be, so for now I limited it to two options:
      • GitHub repo name: spm:nicklockwood/[email protected]
      • GitHub https url: spm:https://github.com/nicklockwood/[email protected]
        Maybe we should add other options by community requests
    2. When we use different formats for the same package repo, they are perceived as different packages:
      mise-spm-name
      I got some errors when trying to change the BackendArg name to generic format (shortened) in SPMBackend::new(name)
      Are there any other options to solve this problem?
  4. Versioning
    Current implementation looking for GitHub release tags for installation candidates. Installation by branch/commit can also be supported, but I'm not sure if this should be done now.

src/backend/spm.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Logic-wise it looks prefect. Nicely done @kattouf.

impl SwiftPackageRepo {
fn new(name: &str) -> Result<Self, eyre::Error> {
let shorthand_regex = regex!(r"^[a-zA-Z0-9_-]+/[a-zA-Z0-9_-]+$");
let shorthand_in_url_regex =
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to generalize this such that it also supports other Git platforms, for example Gitlab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it makes sense globally, I just decided not to spend too much time on unpopular cases right now.

I think we could cover almost everything else cases by implementing:

  1. support for any URLs up to .git
  2. support for various git revisions as versions: tag, commit, branch

and divide the versioning flow into:

  1. github releases
  2. specific git revision

But is it worth doing this right away, I'm not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be done in a follow-up PR. It seems a good candidate to be extracted into a separate function that can be reused across the backend. I'm sure @jdx has already put some thoughts there.

src/backend/spm.rs Outdated Show resolved Hide resolved
struct PackageDescriptionProduct {
name: String,
#[serde(deserialize_with = "PackageDescriptionProductType::deserialize_product_type_field")]
r#type: PackageDescriptionProductType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what's the meaning of this r#type annotation? (I'm not that versed at Rust)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's raw identifier, used to define user data names, same as language keywords (like `type` in swift)
https://stackoverflow.com/a/69871965

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! Thanks for the reference to Swift, it helped :)

mise use swift

assert "mise x spm:nicklockwood/[email protected] -- swiftformat --version" "0.53.10"
assert "mise x spm:https://github.com/nicklockwood/[email protected] -- swiftformat --version" "0.53.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, with pleasure! But the installation time seems too long for testing, don't you think?
Maybe we could to this with some install specific executable feature, but now it takes about 5m and looks not tests friendly.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

5m is terrible! So feel free to skip it then :)

kattouf and others added 2 commits June 3, 2024 15:20
Co-authored-by: Pedro Piñera Buendía <[email protected]>
@kattouf
Copy link
Contributor Author

kattouf commented Jun 3, 2024

@pepicrft Could you also check Requires attention section in PR description? None of this seems like a blocker to you right now?
We have already partially discussed points 3 and 4, so you can focus on 1, 2 and 3.ii

@pepicrft
Copy link
Contributor

pepicrft commented Jun 3, 2024

  1. Missed swift dependency

I'd be consistent with what the other backends do. If they install the required toolchain absent, we can error out telling users that "Swift was not found in the environment" until we install it ourselves. This is a scarce scenario because most developers using this backend will likely be Xcode developers with the toolchain available in their systems.

  1. Support for build artifacts on other platforms

I'm in a similar spot here. At least on Linux, if everything is statically linked, the binary should work, so the issues will likely arise when using dynamic dependencies, which I'm not entirely sure are supported on Linux. Similarly, this is a bit uncommon, so I'd ship this as it is and iterate on it as feedback comes our way.

  1. When we use different formats for the same package repo, they are perceived as different packages

This is a bit confusing, especially considering that they are referring to the same package. @jdx might be able to help with this one.

@kattouf
Copy link
Contributor Author

kattouf commented Jun 3, 2024

I'd be consistent with what the other backends do.

Currently mise doesn't automatically install backend dependencies, instead the documentation suggests installing them using mise or any other convenient method and gives an error message:
image
It seems that the backend is a feature that expects that it will be used by those people who already have the entire necessary tools, they just want to describe them in a single config

Similarly, this is a bit uncommon, so I'd ship this as it is and iterate on it as feedback comes our way.

Agree 🤝

@pepicrft
Copy link
Contributor

pepicrft commented Jun 3, 2024

Overall, fantastic work @kattouf 👏🏼. As one of the advocates of Mise in the Swift ecosystem, I couldn't be more excited of this work landing.
We have to wait for @jdx to do the merge.

@jdx jdx merged commit 8d37aab into jdx:main Jun 3, 2024
10 checks passed
@kattouf kattouf mentioned this pull request Jun 4, 2024
3 tasks
triarius pushed a commit to triarius/mise that referenced this pull request Sep 18, 2024
* first simple implementation

* get executables list and work with them only

* rename SpmForge -> SPMBackend

* Package description deserialization with serde

* copy build artifacts

* refactoring

* spm e2e tests

* use project git client

* refactoring

* fix repo url name usage; fix git checkout

* update test assert

* fix lint violations

* fix package version in e2e test

* add spm repo parsing unit tests

* Better error message

Co-authored-by: Pedro Piñera Buendía <[email protected]>

* reformat

---------

Co-authored-by: Pedro Piñera Buendía <[email protected]>
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.

3 participants