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

Remove the ability to configure lockfile location. #5335

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Remove the ability to configure lockfile location. #5335

merged 1 commit into from
Sep 14, 2023

Conversation

nathanhammond
Copy link
Contributor

  1. Enabling configuration of lockfile location at the global config level means that the lockfile isn't guaranteed consistent across a team. Currently, the only "safe" (read: assured consistent) way to use bun with multiple contributor teams requires setting that value in the local bunfig.toml.

  2. Splitting the two values from each other enables a configuration where you can’t read after writing.

  3. env priority is "correct" for BUN_CONFIG_LOCKFILE_SAVE_PATH but I don't feel like it should exist. It isn't used in the test suite, makes shared configuration impossible, and appears semi-arbitrarily added two years ago in December 2021.

  4. --lockfile introduces the same problem, and since it is per-execution, is extremely difficult to ensure consistency. It also gets removed.


Approach notes:

  • This does not change the peechy schema because I'm ~certain that isn't possible.
  • I don't know what I don't know in Zig.
  • I think there are some other places that can be cleaned up because lockfile_path is now a static string.

Closes #5259

@Jarred-Sumner
Copy link
Collaborator

This makes sense. I think we should do it. I don't know if people are using this flag though

@nathanhammond
Copy link
Contributor Author

This makes sense. I think we should do it. I don't know if people are using this flag though

Amusingly, if any users are using any of these features after this PR they end up with precisely the same experience that a separate contributor trying to work on their repository would have had.

@nathanhammond
Copy link
Contributor Author

nathanhammond commented Sep 14, 2023

I omitted the alternate proposal because I don't think it provides value and introduces complexity, but, for completeness, this is also a valid approach:

Same:

  • Remove --lockfile.
  • Remove BUN_CONFIG_LOCKFILE_SAVE_PATH.
  • Remove install.lockfile.savePath.
  • Remove global config for install.lockfile.path.

Different:
Allow local config (bunfig.toml) for install.lockfile.path with the following conditions:

  1. It is lexically cleaned.
  2. It does not traverse above pwd.
  3. It uses / as separator, even on Windows.

(Turborepo calls this an AnchoredUnixPath.)

Further, during restoration, path.resolve or whatever it is on Zig must be prefixed by the root path. (You can't insert a symlink/Junction into the path and jump out of the directory.)


The difference in code change between the two approaches are extremely limited and it introduces a lot of unnecessary complexity into ecosystem and the mental model.

  • .bunfig.toml and bunfig.toml have distinct sets of available options.
  • You have to teach users what is valid. (What is an AnchoredUnixPath?)
  • All tooling for bun has to implement parsing for bunfig.toml, increasing friction for integration.

@nathanhammond
Copy link
Contributor Author

(Test failures appear to be unrelated?)

@Jarred-Sumner Jarred-Sumner merged commit 8ae9aee into oven-sh:main Sep 14, 2023
15 of 19 checks passed
@nathanhammond nathanhammond deleted the rm-lockfile-config branch September 14, 2023 06:02
paperdave pushed a commit to SuperAuguste/bun that referenced this pull request Sep 18, 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.

install.lockfile.path is unvalidated
2 participants