search

pulsar-edit / ppm

Pulsar Package Manager
MIT License
35 stars 13 forks source link

Update OS, actions, node #57

Closed Spiker985 closed 1 year ago

Spiker985 commented 1 year ago
Spiker985 commented 1 year ago

Needs #53 to hopefully fix the windows CI

Spiker985 commented 1 year ago

image Well there's your problem...

Looks like I'll need to find another way to add a space into that directory lol

Spiker985 commented 1 year ago

...why in the world is Windows 16 using node-gyp -v v9.1.0...don't tell me that having node-gyp hoisted into the package.json is what made Windows install using the correct version :man_facepalming:

Spiker985 commented 1 year ago

I give up. Putting back the hoisted node-gyp entry into the package.json fixed the CI for Windows 16 AND 18 - so I guess it stays there for now...

DeeDeeG commented 1 year ago

I worry about micro-managing this PR, my apologies.

Responding to request for review.

Regarding the CI failure on Node 19: Would personally rather not test any non-LTS Node versions.

I kind of don't want to worry about non-LTS Node. We have plenty of time to integrate those changes after they've been stabilized and packaged for wide consumption, when they appear in Node 20 LTS in the future. There's generally three LTS releases at any given time, that gives us plenty of lead time to get ready for the newest LTS, in my opinion, before being forced to switch. Riding the "Current" release stream is a headache.

Also, commenting out a test feels kind of wrong, I don't think it is genuinely relevant to the changes in .github/workflows/CI.yml. I do expect this test will not work with npm 8 or newer, so its days are most likely numbered, so I suppose removing it here is okay, just seems not neatly scoped to "one PR one change".

And yet we have some much wider-reaching PRs in other repos. So. I dunno if it's so bad...

Again, I don't want to micro-manage, I feel like this started as a draft and I nudged it into being merge-ready by specifying all the details exactly, doesn't feel like good reviewer behavior, but er it (my review) was requested now. Feel free to ping on Discord if you need to talk about it/check in expectations in a less formal way.

Spiker985 commented 1 year ago

The only thing I'd want to see here to improve this PR, which if done my approval still counts, would be to add a small comment at the start of the commented out tests saying why it's commented out, or that just contains a link to this PR, so that developers in the future can see why it was commented out and don't have to do the legwork of finding out or using git blame to do so.

~I'd say that's definitely something I can do, and something I honestly debated about, it mostly just slipped my mind.~ See edit below

I worry about micro-managing this PR, my apologies. Regarding the CI failure on Node 19: Would personally rather not test any non-LTS Node versions.

I'm a-okay with removing 19. I added it just to cover our bases, but I'm also not nearly as familiar with the node ecosystem (really Pulsar and associated repos is my first foray into node outside of dabbling). And it's that lack of familiarity that is driving me to request your guys' reviews. I'd much rather be told "Hey I (don't) think we should do this because of Y obscure thing" than to accidentally cause a regression since we're spread so thin as it is.

Also, commenting out a test feels kind of wrong

~I agree in most instances, but in this particular one I feel it's better to be decisive. Plus, we can revert if we do find it to be required. Normally I also don't want to make a CI pass just for the sake of making the CI pass, but it felt like the test was being a blocker when it was unnecessary, rather than preventing a bad build from going out.~

Something to note, is that although we had previously determined that node-gyp being hoisted was just for that test, it actually makes the CI pass on 16 and 18. Where without it, Windows-2019 tries to use the global node-gyp which is 9+.

~I'll go ahead and add comments to the spec to say "Why" we commented it out, and then if we decide to revisit it in the future, we'll have record - sound fair?~

Edit: Technically the whole reason I wanted to update that test in the first place was due to the fact that I wanted to remove the hoisted node-gyp entry. Since that hoisted entry is required for Windows for node 16 and 18 on 2019 anyways, I should be able to uncomment it (and if it passes) we can actually drop that chagne entirely from this PR