Use corepack to run package managers

[SvenW]

This makes it possible to run NodeJSToolProcesses as interactive processes. Would be nice if we could setup aliases for each package manager instead perhaps as described here.

Do you have any pointers on how to solve it in another way than this?

Fixes #21205

[krishnan-chandra]

I would in general be wary of this because corepack is being removed from future versions of Node. I don't know offhand what the best solution might be, but I think it might be adding the actual underlying package manager binaries (pnpm, yarn, npm) inside the binaries directory instead of corepack.

[SvenW]

I would in general be wary of this because corepack is being removed from future versions of Node. I don't know offhand what the best solution might be, but I think it might be adding the actual underlying package manager binaries (pnpm, yarn, npm) inside the binaries directory instead of corepack.

Sure. If that's the case I agree. But that feels like a separate task, no? Corepack usage is already there, moving to using the package managers themselves feels like a separate thing

[krishnan-chandra]

That's a fair counter; since we are using corepack today in many other places I think this is fair game too.

[huonw]

Thanks for the contribution and thanks for waiting.

I'm unfamiliar with the details of corepack, the deprecation sounds unfortunate, but as observed, Pants already use it in places, so I'm going to get this in. We can revisit if @tobni or others (maybe @AlexTereshenkov?) have alternative ways to approach this.

[tobni]

Nodejs Corepack distribution being deprecated is something needs fixing, sure. Adding this feature doesn't really complicate that.

To be clear, corepack isn't deprecated, just the node binaries distribution containing corepack.

[huonw]

Just checking before merging: can you expand a bit on why these changed?

(No need to revert/change again, but I'd like to be sure that we don't have to update them again when new versions are released, so that these tests don't depend on external state.)

[SvenW]

Yeah, those are now picked up by the version installed via corepack. I'm not really sure on how we could lock them, when running the activate command for corepack perhaps?

[huonw]

Hm, thanks.

It's definitely unexpected to me that setting --nodejs-package-managers={...} with explicit versions doesn't end up using those exact versions, I think we may need to fix this.

@tobni, any opinions/advice?

[huonw]

ah, looking at https://github.com/nodejs/corepack?tab=readme-ov-file#utility-commands suggests that we could run like, for instance, args=("[email protected]", ...). So, maybe:

1. add a method for getting the ...@... installation specification to PackageManager, e.g.:

   def spec(self) -> str:
       return f"{self.name}@{self.version}" if self.version is not None else self.name

2. invoke the process with args=(pkg_manager.spec(), ...)

[SvenW]

Ah, nice find. I'll try and see if I can get to it later this week

[SvenW]

Just quickly tested this and it seems to work for all package managers except yarn that ends up with this error error Couldn't find a package.json file in "/private/tmp/pants-sandbox-pPJw4m"
Might be connected to nodejs/corepack#334

[benjyw]

Hi folks, what's the status on this? @huonw , could this be merged as-is?

[SvenW]

Hi folks, what's the status on this? @huonw , could this be merged as-is?

I've gotten stuck on other work unfortunately and I didn't manage to get all package managers running with the version specified in the run args. yarn for example got caught on the error I wrote about earlier

[SvenW]

I made some changes so we're able to run package managers on a specific version. I was unable to get it running for yarn so therefor the somewhat ugly fix to disregard that specific package manager for now. Would love any help on getting it running with yarn as well but this will at least keep us from needing to update test versions as often :)