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

[refactor] Simplify our 0install vendor to remove unused features #11015

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

ElectreAAS
Copy link
Collaborator

@ElectreAAS ElectreAAS commented Oct 16, 2024

Our vendored dependency, opam-0install, is our only interface with 0install and doesn't use commands or 'machine groups' at all. Here I remove them entirely, without any feature loss.

The goal is to simplify the codebase such that future modifications are easier to make. In particular, looking at things like #10987, #10838, maybe even help with #10837.

@ElectreAAS
Copy link
Collaborator Author

ElectreAAS commented Oct 16, 2024

Note that this PR includes two commits that run ocamlformat on both the 0install-solver and the opam-0install codebases. Working with a formatted file is a lot easier.
We might want to add those to .git-blame-ignore-revs afterwards.
I could also remove the commits if we deem them unnecessary.

@rgrinberg
Copy link
Member

Thanks for getting started on this. To properly fork these projects and develop them as internal dune libraries, I suggest we take the following preliminary steps:

  1. Move them from vendor to src
  2. Remove the import scripts
  3. Run ocamlformat

Once those 3 are done, the removal of dead code should commence as it will be a lot easier to tell what is being deleted. You followed the same steps but in a different order, which makes review a little bit harder. It's not a big deal though, as the changes are still simple. However, it would be helpful to push our changes after preliminary work

@ElectreAAS
Copy link
Collaborator Author

Thanks for the pointers @rgrinberg, I'll work on cleaning up my butchery after stabilizing my changes, I have other ideas still

Relatedly, my last commit pretty much solves #10987, although in a very opiniated way.
I'll look into patching that on its own though

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
…ate scripts

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS
Copy link
Collaborator Author

My above comment is now outdated, this PR is ready for review. The commit solving the weird errors is in another PR.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg rgrinberg merged commit a175dda into ocaml:main Oct 31, 2024
26 of 27 checks passed
@ElectreAAS ElectreAAS deleted the butcher-0install branch November 4, 2024 07:54
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