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.

Sign up for GitHub

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

Jump to bottom

feat: add support for bun.lock #379

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: add support for bun.lock #379

wants to merge 11 commits into from

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Jan 12, 2025 •
edited
Loading

As part of improving ergonomics, Bun has introduced a new text-based lockfile which this adds support for extracting.

While the blog post claims that it's "JSON with comments" (aka JSONC), it's actually "JSON with comments and trailing commas" (aka JWCC or HuJSON) - since the Go standard library only supports parsing standard JSON, I've had to bring in a third-party library to handle parsing, which is designed to leverage the existing standard library by instead handling "standardizing" the input into valid boring JSON.

Aside from the general question of if this library is acceptable to use here, it also seems to require Go 1.23 resulting in the addition of a toolchain line in go.mod which I've never managed to quite figure out what the right thing to do with - overall, I'd like someone from Google to confirm what library they'd prefer we use here. (I've since switched to using a library that requires Go 1.16)

I have also specified the testdata fixtures as JSON5 as that's the most appropriate format supported by both VSCode and IntelliJ/GoLand, though that technically supports more features like single quotes (which it actually seems like bun does not mind if you use in your lockfile, though it'll always use double quotes itself) - personally I think that's fine, but don't mind renaming the files to be .hujson if folks would prefer.

Resolves google/osv-scanner#1405

@G-Rath G-Rath mentioned this pull request Jan 12, 2025
Open
@erikvarga
Copy link
Collaborator

It's generally fine to add new libraries if we think they can't be implemented easily on our side. Can you double check that this new import doesn't cause the SCALIBR binary size to get too much bigger?

Regarding the Go version, do you mean that the lib needs Go 1.23 or beyond? We use Go 1.24 internally so requiring 1.23 or beyond should be fine for us.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 13, 2025

Regarding the Go version, do you mean that the lib needs Go 1.23 or beyond? We use Go 1.24 internally so requiring 1.23 or beyond should be fine for us.

Yes that's my read of it - I think bumping the Go version could be a breaking change but either way probably best done in a dedicated PR.

I'm happy to open that and we can continue any related discussion over there

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 13, 2025

Can you double check that this new import doesn't cause the SCALIBR binary size to get too much bigger?

osv-scalibr on  main [?] via 🐹 v1.22.5 via  v20.11.0
❯ du scalibr-old scalibr-new scalibr-alt
48396   scalibr-old
49512   scalibr-new
49304   scalibr-alt

The scalibr-alt version is built with tidwall/jsonc which was another library I found that could work - I went with the current library to start as it's maintained by tailscale and looks to be actively maintained, but I don't think there's a huge risk if we wanted to use this alt library (which would also be a little nicer as it doesn't return an error, so we'd have one less condition branch)

@G-Rath G-Rath requested a review from erikvarga January 13, 2025 18:39
@G-Rath
Copy link
Collaborator Author

G-Rath commented Jan 13, 2025

@erikvarga I have gone ahead with switching out the library since tidwall/jsonc only requires Go v1.16, so this means I can get CI green but happy to switch back to the tailscale library if you're prefer (or look at other alternatives, as there's a couple out there)

@erikvarga
Copy link
Collaborator

@erikvarga I have gone ahead with switching out the library since tidwall/jsonc only requires Go v1.16, so this means I can get CI green but happy to switch back to the tailscale library if you're prefer (or look at other alternatives, as there's a couple out there)

Hm, if using tailscale means we have to wait until Go 1.24 is released so we'll stay compatible with the last 2 versions then maybe we're better off using tidwall/jsonc.

@G-Rath G-Rath mentioned this pull request Jan 15, 2025
Merged
@G-Rath G-Rath requested a review from erikvarga January 15, 2025 18:45
@G-Rath G-Rath mentioned this pull request Jan 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RiskyMH RiskyMH RiskyMH left review comments

@erikvarga erikvarga erikvarga approved these changes

@another-rex another-rex Awaiting requested review from another-rex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

REQUEST: Support bun.lock lockfile used by Bun
3 participants
@G-Rath @erikvarga @RiskyMH