mina icon indicating copy to clipboard operation
mina copied to clipboard

Remove unnecessary submodules (mini version)

Open georgeee opened this issue 10 months ago • 10 comments

Mini version of #15501.

Explain your changes:

  • Remove infrequently updated submodules and copied-in repositories (except for ocaml-rocksdb, which building strategy makes it non-trivial)

Explain how you tested your changes:

  • [x] nix build 'git+https://github.com/MinaProtocol/mina?branch=georgeee/remove-unnecessary-submodules-mini&submodules=1#devnet' to test nix build
  • [x] CI passes

Checklist:

  • [x] Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • [ ] Modified the current draft of release notes with details on what is completed or incomplete within this project
  • [x] Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • [x] Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • [x] All tests pass (CI will check this if you didn't)
  • [x] Serialized types are in stable-versioned modules
  • [x] Does this close issues? None

georgeee avatar Apr 11 '24 15:04 georgeee

!ci-toolchain-me

georgeee avatar Apr 11 '24 15:04 georgeee

!ci-build-me

georgeee avatar Apr 11 '24 16:04 georgeee

!ci-toolchain-me

georgeee avatar Apr 19 '24 11:04 georgeee

!ci-build-me

georgeee avatar Apr 19 '24 12:04 georgeee

!ci-build-me

georgeee avatar Apr 19 '24 14:04 georgeee

!ci-build-me

georgeee avatar May 09 '24 17:05 georgeee

So the issue is the following:

git checkout origin/berkeley
./nix/pin.sh # just pins the submodules
git checkout origin/develop
./nix/pin.sh
git status

will give following output for the last command:

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        src/external/

Repeating the steps:

git checkout origin/berkeley
./nix/pin.sh # just pins the submodules

removes this status, as expected.

If we switch back to develop and try to build the stuff:

git checkout origin/develop
./nix/pin.sh
nix develop mina
make build # from within nix shell

It succeeds though.

georgeee avatar May 09 '24 18:05 georgeee

I suggest the following two remedies to avoid unnecessary friction:

  1. Add src/external to .gitignore
  2. Add (data_only_dirs external) to src/dune

This way we'll ensure neither git nor dune will ever see src/external even if it happens to exist on file system after the switch of the branch.

georgeee avatar May 09 '24 18:05 georgeee

Main concern brought up by Matthew is use of check_opam_switch.

When berkeley and develop have divergent version version sets of library dependencies, this requires either use of two switches or rebuilding switch on each change of the branch. Which is of course not convenient.

This issue doesn't appear when working from within nix shell, but is a problem otherwise.

The easiest solution that I see is to reject the idea of -mina suffixes. Option --set-default in opam repository add --yes --all --set-default o1-labs https://github.com/o1-labs/opam-repository.git should work just fine without this additional help of -mina suffix. And in nix it's even clearer that o1-labs repo takes precendence.

So -mina suffixes does seem to be completely unnecessary, so reverting this change is just a natural way to resolve the issue.

And also now if one runs:

opam repository add --yes --all --set-default o1-labs https://github.com/o1-labs/opam-repository.git
opam update
opam switch import opam.export

once on feature branch (or future develop) and then switches forth and back between feature branch and berkeley , make check_opam_switch succeeds without any additional action.

georgeee avatar May 13 '24 17:05 georgeee

I'm not sure what the issue actually is at the moment. Is that only that this change would need OPAM users to have 2 switches?

If so, isn't it already an issue today if, says, someone has to update library dependencies in develop. Or do we actually implicitly enforce some restrictions on library dependencies wrt to branching? (equality? subset inclusion master \in compatible \in develop).

rbonichon avatar Jul 11 '24 19:07 rbonichon

I'm not sure what the issue actually is at the moment. Is that only that this change would need OPAM users to have 2 switches?

If so, isn't it already an issue today if, says, someone has to update library dependencies in develop. Or do we actually implicitly enforce some restrictions on library dependencies wrt to branching? (equality? subset inclusion master \in compatible \in develop).

This is an issue we have already, and it would raise for any PR changing the opam.export.

@mrmr1993's suggestion is to introduce some automation with the use of local switches to avoid unnecessary confusion when performing development in non-nix environment.

I guess this PR for now is blocked on us finding a way to auto-manage multiple switches.

georgeee avatar Jul 17 '24 20:07 georgeee

!ci-toolchain-me

georgeee avatar Aug 23 '24 18:08 georgeee

!ci-build-me

georgeee avatar Aug 26 '24 09:08 georgeee

!ci-toolchain-me

georgeee avatar Aug 26 '24 18:08 georgeee

!ci-build-me

georgeee avatar Aug 27 '24 17:08 georgeee

!ci-build-me

georgeee avatar Aug 28 '24 22:08 georgeee