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!: add remote package source to lockfile #297

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

mrcjkb
Copy link
Member

@mrcjkb mrcjkb commented Jan 1, 2025

This PR introduces a RemoteSource type, which contains information about what type of source (e.g. a Luarocks server) and the URL a LocalPackage was pulled from.

The new field has to be an Option, because it's possible to run rocks build on a local project.

@mrcjkb mrcjkb marked this pull request as draft January 1, 2025 18:40
@mrcjkb
Copy link
Member Author

mrcjkb commented Jan 1, 2025

I'm stuck on the fact that the LocalPackage is constructed in the build function, but the RemoteSource struct is pub(crate).
I don't want to make it pub, so I'll start work on #293.

@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from 70b09e3 to b743830 Compare January 1, 2025 18:43
@mrcjkb mrcjkb changed the base branch from mj/push-xyppvtpnxpum to mj/push-yzuwlustwxrp January 3, 2025 13:00
@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from b743830 to f207947 Compare January 3, 2025 13:00
@mrcjkb mrcjkb force-pushed the mj/push-yzuwlustwxrp branch from eb2e112 to ed28a80 Compare January 3, 2025 13:00
@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from f207947 to 9cbdd5a Compare January 3, 2025 14:25
@mrcjkb mrcjkb force-pushed the mj/push-yzuwlustwxrp branch from ed28a80 to ff26454 Compare January 3, 2025 14:25
@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from 9cbdd5a to 1e9cbac Compare January 3, 2025 17:41
@mrcjkb mrcjkb marked this pull request as ready for review January 3, 2025 17:42
@mrcjkb
Copy link
Member Author

mrcjkb commented Jan 3, 2025

Stacked this on all the builder pattern refactors.

This ended up getting a lot bigger than I originally anticipated (sorry for that 😞).

Here's a summary of the changes:

  • Changed Config::server() and ::extra_servers() to return Urls instead of Strings.
  • Introduced a RemotePackageSource type, which provides information about where a remote package was fetched from.
    Currently, there are two variants:
    • LuarocksServer - with a URL
    • RockspecContent - for packages that were built from a rockspec on a local filesystem
  • We serialize the RemotePackageSource and store it in the lockfile [this is a breaking change!].
    • Question: Do we want to base64 encode the RockspecContent, to make it more compact?
      My gut feeling says no, because it wouldn't be good for git diffs.
  • The introduction of RemotePackageSource and its presence as a field in LocalPackage has resulted in a clippy large_enum_variant warning for RockMatches, which is why I have modified it to hold LocalPackageIds instead.
  • Because RemotePackageSource is private, the LocalPackage constructor is now also private, so a bunch of public APIs that used to take a LocalPackage now take a LocalPackageId and look up the package from the Tree (for example, Remove).

@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from 1e9cbac to 397d470 Compare January 3, 2025 18:00
@mrcjkb mrcjkb changed the title feat!: add url to lockfile feat!: add remote package source to lockfile Jan 3, 2025
@mrcjkb mrcjkb force-pushed the mj/push-lyzszxlmlvkk branch from 397d470 to 3535384 Compare January 3, 2025 18:13
@mrcjkb mrcjkb requested a review from teto as a code owner January 4, 2025 18:09
@mrcjkb mrcjkb merged commit da3ae4e into mj/push-yzuwlustwxrp Jan 4, 2025
13 checks passed
@mrcjkb mrcjkb deleted the mj/push-lyzszxlmlvkk branch January 4, 2025 18:09
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.

2 participants