Delete old Name Lookup methods in UCM #5875
Conversation
| cabal-version: 1.12 | ||
|
|
||
| -- This file has been generated from package.yaml by hpack version 0.36.0. | ||
| -- This file has been generated from package.yaml by hpack version 0.37.0. |
There was a problem hiding this comment.
What can we do about this?
- everyone uses nix; kind of a pain, but maybe it's pointless to have only some people using it
- just delete all the .cabal files at the start of the ci build, so that hpack regenerates everything
- what other options?
There was a problem hiding this comment.
Can you explain what sort of problems this is causing folks?
I'd sooner introduce a git hook on my config which sed's it back to the old version rather than use nix haha.
Deleting cabal files also seems fine, though I suppose us as developers might still get the warnings?
There was a problem hiding this comment.
Sorry yeah — an example would be: Alice is using stack w/ hpack 36 and Bob is using stack w/ hpack 37.
Bob makes a change to codebase-sqlite/package.yaml, which causes unison-codebase-sqlite.cabal to be regenerated with the corresponding updates and the auto-generated comment "blah blah version 0.37.0", and it gets merged.
Later, Alice makes another change to codebase-sqlite/package.yaml. She also needs to regenerate unison-codebase-sqlite.cabal, but her hpack 36 will semi-silently fail to do so, because it reads that "0.37.0" comment and decides "this .cabal file may include features that I don't know how to generate, so it's not really safe for me to overwrite it", which means unison-codebase-sqlite.cabal doesn't get Alice's changes.
Depending on specifically what she changed, this might trigger a build failure, but it also might not. e.g. it maybe some optimization setting that doesn't end up happening despite getting merged to trunk.
It doesn't go into effect until whenever Bob happens to make another change to codebase-sqlite/package.yaml, at which point his hpack 37 will finally apply both changes (her latent ones and his new ones).
With my rubber duck in hand, I wonder if there's some flag that would just make hpack 36 in this example just fail louder?
There was a problem hiding this comment.
ChatGPT suggests four solutions.
- a hallucination
.gitignoreand don't commit the generated.cabal; I'm not sure if there's a 1-liner that will do that, without gitignoring the other .cabal files some people have wanted.- a hallucination
- call stack with
--hpack-forcewhich skips the version mismatch checks. This also seems like a good option as we are not using any bleeding edge features at the moment
If 2. then idk how to easily support cabal users. They'd have to run hpack locally.
There was a problem hiding this comment.
Yeah, 2 breaks cabal users, if 4 works then that that sounds great to me, we always want the cabal files to match the package.yamls anyways.
There was a problem hiding this comment.
Okay, let's plan to implement 4 at some point. Hopefully it's a real thing and not a hallucination.
Given that we haven't depended on them in a long time, what would you say are the chances that they are actually still correct? Even if they weren't silently broken due (only?) to lack of appropriate sqlite indexes? |
I don't know that the code was ever correct for directly using in UCM since these were designed for Share, it'd need tweaks either way. |
I guess I'm wondering whether ... if you/we decided we wanted to do something like this in UCM, would we start with this code, or just write it from scratch anyway, or would we write it from scratch but look at this code while doing it? If it's either of the latter two, then we can delete it and leave a searchable note in the PR description. |
Yeah I'm game to delete it, probably best for keeping code simpler and best not to bother compiling code that's unused. Gotta be a little careful because there are migrations referencing some of it, but it shouldn't be too bad. If that works for you let me know and I can get that done next time I'm waiting for something else to build haha. |
|
Yeah I'm fine with deleting it if you and @mitchellwrosen are fine with it. |
1bdb940 to
c90ec8a
Compare
|
@aryairani okay if this builds 🤞🏼 then it should be good to go :) |
Overview
I accidentally tried to use one of these methods for a feature and was confused why it didn't work 🙃 ; figure it's best to clearly deprecate them.
History lesson: way back when we were first splitting of Share it used a Sqlite DB and all the same UCM endpoints for serving codebase traffic.
As part of making it efficient I added a bunch of utilities and indexes for better searching codebases; these were implemented in
unison, but were only used by the Share instances, the indexes themselves were never actually maintained in UCM codebases.Nowadays Share doesn't use any of these utilities, it's all migrated to postgres.
Implementation notes
Test coverage
Nah.