package_control_channel PR tests still depend on packagecontrol.io

[braver]

Current a test for a channel update PR sends requests to the packagecontrol.io website. We cannot update the logic that runs there and it's flaky and outdated.

There are new actions that could replace most of what it does. The LSP project uses them. They're probably already overly complex and a bit outdated, but it's a good starting point.

The most important things are guarding validity agains the schema for all changes, and making sure new package additions don't cause problems that aren't easy to spot manually.

It is not a goal to add comments to the PR's or to add a reviewer bot or anything. Scripts that output to stdout would be fine.

For the time being an additional constraint is that we don't have access to the package_control_channel to add for instance secrets.

[braver]

@kaste @FichteFoll

As the OP explains, reviews of new packages still depend on the old site. And the script that lives on the old site is outdated and not under our control. The setup is outdated too, there is no need for that to live on a site.

I've made a version based on the review script and GitHub action that the LSP project uses, at packagecontrol/st-package-reviewer-action#5. It seems to work, so I'm going to try and put that to use.

There remains two serious issues with it though:

• It fully vendors an outdated version of the Package Control client package, for its ability to crawl and download a package. There is probably a better way to do this. In most cases it's not even necessary to do all this, the underlying package-review script (also vendored) can also review git repos (ie. non-packaged code) and that's perfectly fine for 99.9% of packages.
• It's not great at resolving more complicated changes to the repo than a single package addition. It gets confused over which packages are added or removed and what packages to then test. See add ColorConverter wbond/package_control_channel#9178. I tested the script on that situation and just like in that PR it mistakenly thinks ColorHints is removed, and doesn't notice that ColorConverter is added (and thus forgets to test the new package).

So while it looks like moving the tools to a place where we own them is doable, the tools themselves are problematic and will need work at some point. It's way beyond what I know how to do, so I'm hoping someone else is inspired to have a look.

[kaste]

You asked me once to help probably with that. But the PR you link to I can't read really as that's not my project. Of course, I can, given a PR, read the registry changes, (new, changed, removed packages), and then run thecrawl on these and for example extract zipped packages. (And then some script uses these zips fror the actual review.) But I wouldn't know how that fits here exactly, i.e. how to wire that up.

[braver]

Yeah, I get that PR is a bit opaque. It took me ages to figure out what's what. What you're saying is the gist of it though, so looks like you already mostly understood it. So maybe ignore that PR and that repo, we'd build something new anyway. A new repo.

1. Something that can "read the registry changes, (new, changed, removed packages)"
2. then "run thecrawl on these and for example extract zipped packages"
3. then feed each package zip into st_package_reviewer, see usage
4. and finally prints any notices, errors and warnings in accordance with GitHub's annotation format

4 is in a nutshell what packagecontrol/st-package-reviewer-action#5 does while leaving 1-3 in place. The bits that do 1 and 2 need replacing. Effectively what we need is a new wrapper around st_package_reviewer that we can use in PR reviews of submissions to any package registry (the default one, LSP, SublimeLinter).

Now st_package_reviewer itself also needs work. But those are issues on that repo. The most urgent ones I can handle probably. We can for now assume that that component itself is fine.

It looks like that PR of mine will actually work. If that's true there is no immediate urgency besides it being annoyingly buggy and impossible to maintain 😅 It's fine for 90% of cases and that's good enough. I'm just hoping to spark some inspiration in a few people that might have the chops to build something like that, because I certainly can't.

---

To be clear, packagecontrol/st-package-reviewer-action#5 will close this issue as it was originally stated, it will remove our dependence on the packagecontrol.io website and put us in full control of everything. We're now just discussing what the future of this should look like.

[kaste]

Q: Why do we run thecrawl on new (or changed) packages?

thecrawl is not designed to give a lot of useful output if the registry definition is wrong. It is actually designed around the premise that the registry is somewhat well-formed and for example falls back if no tags are found.

Q: Should we really work on the zips thecrawl finds or rather extract the tip of master/main?

During the review, users would need to re-tag every tip so that the checker even sees the upgrades. (They can re-use the same tag however.)

[braver]

During the review, users would need to re-tag every tip so that the checker even sees the upgrades

Yes, that's definitely a frustration in the current setup.

The only reason to run the reviewer on packages instead of the tip of the default branch is in case a package needs a build step. Ie. if the package is not simply provided as a tag on the repo that's in the details key. The use cases we're talking about, LSP, SublimeLinter, default channel, we see a submission like that perhaps once every 2-5 years. And we typically advise against it if it can be avoided at all (because it obscures the actual code that will be running on people's machines).

I would definitely be open to a more pragmatic approach that simply correctly resolves the diff in the registry and feeds git repository urls into st_package_reviewer, because it can handle that too.

[kaste]

How can we review packages that need a build step? I thought these were the closed source ones.

[braver]

Even a closed source project needs to deliver a package of code that Sublime can use, and therefore the reviewer tool can inspect for validity and compliance. Indeed, we can't review the code manually, not without effort at least, but the tool could. I think that's perhaps why it works the way it works now.

More recently I think our position is shifted towards the default channel not necessarily being the means to deliver such packages ... users want to be able to see what gets put on their machine for one reason. We're not really open to adding new external repositories anymore. So closed source might be besides the point.

However, I've seen cases with open source code too though. For instance, a base syntax definition that gets converted into 4 separate syntaxes with a build step. Or, historically, a syntax definition in JSON that would get converted to tmLanguage.

... again, these things don't come along very often and I'm not sure they're valid anymore. It's a different world compared to 10 years ago.

So, reviewing packages is for me not a goal. Reviewing git repository HEAD's is probably even better.

[kaste]

Think about it, ask around, and tell me which variant you want (or both?).

[braver]

Ok, I'll open a discussion so we can brood on it for a while.