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

Bun support #290

Merged
merged 13 commits into from
Oct 22, 2023
Merged

Bun support #290

merged 13 commits into from
Oct 22, 2023

Conversation

harrel56
Copy link
Contributor

@harrel56 harrel56 commented Oct 3, 2023

Hi, I was thinking about developing a standalone bun gradle plugin, but stumbled upon this issue (#288) and thought that I can give it a try.

For now, it's WIP but I tested it out and installing bun and running it somewhat works. Still, there probably will be a lot more stuff to be done. If you have any suggestion/pointers I will gladly take any advice - at this state it's basically a copy of pnpm/yarn setup.

And there is one major issue: bun is not yet supported on windows :( Normally I would work around it by using WSL but I'm not sure how's that gonna play with the rest of the code. So I guess windows support would be out of scope in this PR

@deepy
Copy link
Member

deepy commented Oct 4, 2023

At a glance this looks good, but we're gonna need a test to verify the functionality, I think adapting result1 and result2 from this test should get you most of the way
https://github.com/node-gradle/gradle-node-plugin/blob/master/src/test/groovy/com/github/gradle/node/npm/task/NpmTask_integTest.groovy#L15

@harrel56
Copy link
Contributor Author

harrel56 commented Oct 4, 2023

Yeah, gonna add tests for sure. Also gonna add BunInstallTask and BunxTask. Should I add task rules for bun too?

@deepy
Copy link
Member

deepy commented Oct 4, 2023

I'd skip the task rules, the personal experience I've got with them is that they're good for build engineers (who don't like them) and absolutely awful for average users (who loves them)
The short of it is that it makes people expect npm_install to be the correct task and then they get unexpected results

@harrel56 harrel56 marked this pull request as ready for review October 10, 2023 19:09
@harrel56
Copy link
Contributor Author

Alright, I think this should be somewhat complete. I would expect that some GH actions could fail on different OSes - if that happens I'm gonna try fix it asap. Let me know if there's anything else to be done :)

@harrel56
Copy link
Contributor Author

Also all the Install tasks are kind of funky - personally would separate BunInstall & BunCi (which is just install with --frozen-lockfile), but I don't know if it would make sense in a real world CI/build flow. Ofc anyone could write the Install task by themselves in a way that it suits them, so I guess it's not a big deal

@deepy
Copy link
Member

deepy commented Oct 17, 2023

Conditional tasks gets a little weird when you want to depend on them, and from experience it seems most people use the install tasks anyways (mostly with ephemeral environments)

I'm gonna have time to review this properly on Sunday or during next week, but at a glance it looks good, if it's merged I may change the API.
But there's one major outstanding question, given that the Windows support doesn't yet exist in Bun do you imagine that people using this would expect to wire up potential Windows support themselves or want a convenience-method for doing the wiring?

@harrel56
Copy link
Contributor Author

harrel56 commented Oct 17, 2023

But there's one major outstanding question, given that the Windows support doesn't yet exist in Bun do you imagine that people using this would expect to wire up potential Windows support themselves or want a convenience-method for doing the wiring?

I have a hard time imaging how it could work on Windows and how users could make bun tasks work. So for now if a project uses any of bun related stuff gradle builds will just fail. The only thing I can think of is providing "temporary" Windows support via WSL2. But it would have a few quirks:

  • all that probably guarded by some feature flag on NodeExtension to ensure smoother change when official bun support for Windows finally lands
  • each bun command would be run with something like this: commandLine 'cmd', '/c', 'wsl', '-e', 'bash', '-li', '-c', '"bun install"'
  • if there is no WSL installed then there's really nothing we can do about it - so it will just fail
  • installing bun via npm isn't really an option - you can have no node and npm on WSL so it would be required to run all setup tasks on WSL too. Honestly I don't see going in that direction

My suggestion would be to make it as simple as possible: if before-mentioned feature flag is turned on then we will assume that user has WSL installed as well as bun (on the WSL). So BunSetup task would be skipped in that case.

Let me know what you think about it, I could do a follow up PR with such "experimental" support.

PS I'm a Windows user so it's feasible to develop it on my end, but I don't know how it will look in terms of testing on CI

@deepy
Copy link
Member

deepy commented Oct 22, 2023

We'll skip initial Windows support, WSL support might be useful on its own (in general for the plugin), but is a major change (code-wise) and can wait :-)

@harrel56 harrel56 changed the title WIP: Bun support Bun support Oct 22, 2023
@deepy
Copy link
Member

deepy commented Oct 22, 2023

From an initial glance this looks good, I'm looking into the test failures and some of them are caused by tests that assume local binaries being available (which is ok, I'll fix that on CI later)
There's some slight cleanup that needs to be done, but I can do that after merging

@deepy
Copy link
Member

deepy commented Oct 22, 2023

Okay there's some little-bit-more-than-slight cleanup that needs to be done as well, I'll work on that after merging but I'm gonna list the things here before extracting them to a new issue

  • Verify the tests
    • Verify that the tests actually test bun
    • Check that the test cases use either a downloaded or local copy of bun
    • Use the same bun version in tests that download bun
  • Verify manually that download = true bun works
  • Verify manually that download = false bun works
  • Check if anything from BunExecRunner can be shared between the other ExecRunners

@deepy
Copy link
Member

deepy commented Oct 22, 2023

Most of that is because the general test setup in gradle-node-plugin is unwieldy :-)

@deepy deepy merged commit 16ecc0f into node-gradle:master Oct 22, 2023
2 of 6 checks passed
@deepy deepy mentioned this pull request Oct 22, 2023
8 tasks
@deepy deepy self-requested a review October 22, 2023 12:02
@deepy deepy mentioned this pull request Oct 23, 2023
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