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

Redesign registry; Allow publishing to registry #86

Merged
merged 46 commits into from
Jun 3, 2024

Conversation

kubouch
Copy link
Contributor

@kubouch kubouch commented Mar 26, 2024

Description

This changes registry to from one file that is manually updated to multiple files that are auto-generated. Now, the registry should be only updated with the new nupm publish command.

See the doc page for more details and the registry/ directory for the new proposed structure.

Misc changes

  • The structure of the registry is also different. The git/local package type is now encoded in a flat table instead of being a separate record. See registry/ and tests/packages/registry/ for examples.
  • The path field can be null, not requiring the "."

TODO

  • Add package file hashing to stop redownloading them all the time

@kubouch kubouch marked this pull request as ready for review March 28, 2024 16:56
@kubouch kubouch requested a review from amtoine March 28, 2024 16:56
@kubouch
Copy link
Contributor Author

kubouch commented May 4, 2024

One argument for keeping the nuon files "raw" is that they do not contain newlines, so their hashes should be the same on Windows/non-Windows. If we made the nuon files more human-friendly, they would contain newlines which could potentially mess things up: Simply opening and closing a nuon file could change the line endings in the entire file, making its hash not match the expected hash. Maybe it would be fine, but we'd have to make sure and test it, I'd rather avoid this by having no newlines, at least for now.

@texastoland
Copy link

Just devil's advocate but could you manually replace all \r with empty string before saving?

@kubouch
Copy link
Contributor Author

kubouch commented May 4, 2024

Just devil's advocate but could you manually replace all \r with empty string before saving?

Yes, that's an option (or convert all \n to \r\n). I chose to side step it entirely for now. I'm a bit afraid that once we start messing with newlines, we'd open a can of works. Like I mentioned above, I can imagine just viewing the file in some smart editor could convert the line endings automatically. Then there is also Git which can convert the line endings depending on user's preferences. It's not something I want to mess with right now 😆 .

@amtoine
Copy link
Member

amtoine commented May 5, 2024

nice job @kubouch !!

Windows is such a pain...
in nu-git-manager, i wrote a path sanitize command that makes Windows path better (lol) and i have to use it almost on all paths 🤣

amtoine
amtoine previously requested changes May 5, 2024
Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

the new commits look fine to me 👌

there's just a review question and some missing packages 😉

use std assert

def make-it-similar []: table<name: string, version: string, url: string, revision: string, path: string> -> table<name: string, version: string, url: string, revision: string, path: string> {
    sort-by version name | select name version url revision path
}

let old = http get https://raw.githubusercontent.com/nushell/nupm/main/registry.nuon
    | get git
    | make-it-similar

let new = open registry/registry.nuon
    | get path
    | each {|it| open ("registry/" | path join $it)} # open each registry package file
    | flatten                  # extract the path and revision from `$.info`
    | update info { flatten }  # |
    | flatten --all            # |
    | reject type
    | default "." path         # remove the `null`
    | make-it-similar

if $new != $old {
    $new | to nuon --indent 4 | save --force /tmp/new.nuon
    $old | to nuon --indent 4 | save --force /tmp/old.nuon
    diff -u /tmp/old.nuon /tmp/new.nuon
}
print "all good"

gives me

--- /tmp/old.nuon       2024-05-05 13:15:41.093046938 +0200
+++ /tmp/new.nuon       2024-05-05 13:15:41.093046938 +0200
@@ -245,55 +245,6 @@
         .
     ],
     [
-        nu_plugin_explore,
-        "0.92.0",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.92.",
-        .
-    ],
-    [
-        nu_plugin_clipboard,
-        "0.92.1",
-        "https://github.com/FMotalleb/nu_plugin_clipboard",
-        "98d7102",
-        .
-    ],
-    [
-        nu_plugin_explore,
-        "0.92.3-2595f31541554c6d8602ebebc9dffbc4dd29dd89",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.92.3-2595f31541554c6d8602ebebc9dffbc4dd29dd89",
-        .
-    ],
-    [
-        nu_plugin_explore,
-        "0.92.3-55edef5ddaf3d3d55290863446c2dd50c012e9bc",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.92.3-55edef5ddaf3d3d55290863446c2dd50c012e9bc",
-        .
-    ],
-    [
-        nu_plugin_explore,
-        "0.92.3-be5ed3290cb116cbe9f3038f7a2d46aaac85a25c",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.92.3-be5ed3290cb116cbe9f3038f7a2d46aaac85a25c",
-        .
-    ],
-    [
-        nu_plugin_explore,
-        "0.92.3-c06ef201b72b3cbe901820417106b7d65c6f01e1",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.92.3-c06ef201b72b3cbe901820417106b7d65c6f01e1",
-        .
-    ],
-    [
-        nu_plugin_explore,
-        "0.93.0",
-        "https://github.com/amtoine/nu_plugin_explore",
-        "0.93.0",
-        .
-    ],
-    [
         nu_plugin_port_scan,
         "1.0.10",
         "https://github.com/FMotalleb/nu_plugin_port_scan",

related to

nupm/publish.nu Outdated Show resolved Hide resolved
kubouch and others added 2 commits May 5, 2024 15:35
@kubouch
Copy link
Contributor Author

kubouch commented May 5, 2024

Ah yeah, it's missing the added packages. I currently don't have time to add them. If you have time, you can merge it and publish them again to the registry using the new nupm publish. This way it would get tested by someone apart from me. Otherwise, I can do it whenever I find the time.

these versions have been removed in a merge conflict resolution and were
defined in nushell#88.

i did run the following command after checking out on the different
revisions:
```nushell
use /path/to/nupm/nupm
nupm publish /path/to/nupm/registry/registry.nuon --save --git
```
@amtoine
Copy link
Member

amtoine commented May 8, 2024

no worries 👍

i was able to add most of them apart from

  • nu_plugin_explore on 0.92.0:7f74017 because "Version 0.1.2 of package nu_plugin_explore is already published"
  • nu_plugin_clipboard on 98d7102 because "Version 0.91.0 of package nu_plugin_clipboard is already published"

@kubouch
Copy link
Contributor Author

kubouch commented May 8, 2024

Now thinking about it, I think we should support only semver-style versions because we need to be able to sort by version and select the latest. The same with the main version. At least in the official registry, having random strings would be too confusing. I understand the value of having, e.g., the main as an always up-to-date package, but it really messes up the version resolution.

I'd propose keeping only semver in the official registry, but let's keep experimenting with being able to have some alternative versions. These could be allowed e.g. only in your personal registry, but not for publishing to the official one. Or the version field could be null. Not sure...

@texastoland
Copy link

I think we should support only semver-style versions

Commit hashes are supported in semver (in case it helps).

@kubouch
Copy link
Contributor Author

kubouch commented May 8, 2024

TIL, but I still think we need to restrict our versions to a subset that doesn't prevent sorting. Otherwise, we wouldn't be able to reliably update a package. You want to type nupm update nu_plugin_explore and get the latest version. Now, which one of the four different 0.92.3s is the newest?

We could support having a null/main/latest in place of the version. In that case, this version would always be considered “newest” and would be always re-fetched when you call nupm update. nupm install would also always install this version if you don't specify the version manually. This would be useful for cases like the current tmux-sessionizer tracking the main branch. That would be OK for a private / unofficial registry. For the official registry, we'd disallow it.

@texastoland
Copy link

I still think we need to restrict our versions to a subset that doesn't prevent sorting.

I trust your vision for it ✌🏼 FWIW the spec is major.minor.patch-alphabetical+ignored.

@amtoine
Copy link
Member

amtoine commented May 10, 2024

i agree @kubouch, maybe these deps are simply ill-defined 👀

does that mean that

  • the two i cannot add shouldn't be added?
  • the ones that are not proper semver should be removed?

then we can land this PR 🙏

@kubouch
Copy link
Contributor Author

kubouch commented May 25, 2024

@amtoine I republished the problematic packages, should be good now. The problem with nu_plugin_explore is that revisions up to 0.93.0 still had the old version 0.1.2 in their nupm.nu, so they're not publishable because 0.1.2 is already published. Seems like you forgot to update it.

@amtoine
Copy link
Member

amtoine commented Jun 1, 2024

aaah yeah, you're right 😭

we're good to land then? i kinda lost track of whole the changes lol

@amtoine amtoine dismissed their stale review June 1, 2024 10:47

appears to be ok now?

@amtoine
Copy link
Member

amtoine commented Jun 1, 2024

still think the registry diffs are impossible to review, which is a bit of a bummer imo 😕

@texastoland
Copy link

Same. I think there'd be some value in standardizing on Unix line endings. Otherwise DX will be a step above binary. Epic regardless 👏🏼

@kubouch
Copy link
Contributor Author

kubouch commented Jun 3, 2024

Thinking about it, I think what we can do is to make nupm publish generate one type of line ending (LF or CRLF), but more importantly, we can change the hash function to replace all CRLF with LF in text files and hash that. That should give us identical hashes on Windows and Unix.

I'm going to merge this as-is, but I'll play with the newlines in the next PR, or anyone else can take a shot at it. I think relying forever on having zero newlines doesn't sound very robust in the long run.

@kubouch kubouch merged commit 42d65a9 into nushell:main Jun 3, 2024
3 checks passed
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