-
Notifications
You must be signed in to change notification settings - Fork 244
Support revisions and more #1775
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
Support revisions and more #1775
Conversation
bors try |
tryBuild failed: |
Did you check that this:
Does not trigger unwanted rebuilds? I can't help worrying that this will include the hash of the plan in the hash for the library derivation itself. Does every change to the plan result in a rebuild of I think we discussed encoding the |
Yikes. This sounds a bit scary. The thing I would naturally think to do in this case is to get cabal to use a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a good sense of whether this makes sense. The main thing that makes me a bit nervous is the curl hack.
lib/cabal-project-parser.nix
Outdated
inherit parseIndexState parseSourceRepositoryPackages parseRepositories | ||
# These are only exposed for tests | ||
parseSourceRepositoryPackageBlock parseRepositoryBlock; | ||
inherit parseIndexState parseSourceRepositoryPackages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can your tool also spit out at least the index-states? would that let us fix the problem with us parsing them wrong? I guess the s-r-ps are harder because we rely on the magic comments...
overlays/haskell.nix
Outdated
@@ -540,7 +494,6 @@ final: prev: { | |||
evalPackages = final.lib.mkDefault evalPackages; | |||
inputMap = final.lib.mkDefault inputMap; | |||
} ]; | |||
extra-hackages = args.extra-hackages or [] ++ callProjectResults.extra-hackages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you purge this from everywhere? since now it seems like it's totally unsupported
It seems to only include the hash of the cabal file itself, which (I guess) would be fine.
To test this I made a simple empty package built it adding {
- "/nix/store/gvx4nv5bab8bhkf95vc8qzh6r73pblla-tmp-project2-exe-tmp-project2-0.1.0.0.drv": {
+ "/nix/store/xmr2ihs54kcs3a0zsrjyfy9mdgzsfvx5-tmp-project2-exe-tmp-project2-0.1.0.0.drv": {
"outputs": {
"data": {
- "path": "/nix/store/a9r9pdd0kfh0ibwm8xlqc8sk1ps97a8h-tmp-project2-exe-tmp-project2-0.1.0.0-data"
+ "path": "/nix/store/b65sarw7gnjq4aqwnvqxz9x05k8iaj7v-tmp-project2-exe-tmp-project2-0.1.0.0-data"
},
"out": {
- "path": "/nix/store/sf3wpjw5p52gdf4lcxai2jx6xaplnjhs-tmp-project2-exe-tmp-project2-0.1.0.0"
+ "path": "/nix/store/ihf8zzwzqx7shl6nqii5d78zgl4dmd3a-tmp-project2-exe-tmp-project2-0.1.0.0"
}
},
"inputSrcs": [
- "/nix/store/zhmajpzng3jjzqfjrs05nfrd8qknbbqy-source-root-exe-tmp-project2"
+ "/nix/store/0zrc9ynjpvfwsa9r9l3kisngxm2k5hib-source-root-exe-tmp-project2",
],
"inputDrvs": {
"/nix/store/3p8lp2fzr30nws00x811lg7k9z5axg45-glibc-locales-2.35-163.drv": [
"out"
],
- "/nix/store/4lcnpwddj2ii5dj01j5djn48k90jl80i-tmp-project2-exe-tmp-project2-0.1.0.0-config.drv": [
- "out"
- ],
"/nix/store/f6k3g1wyzb3wk9rdvq60ifkd7pxvxzwa-bash-5.1-p16.drv": [
"out"
],
@@ -28,13 +25,19 @@
"/nix/store/i25q6bwyb6xjr59vbly2yr4785i0i977-remove-references-to.drv": [
"out"
],
+ "/nix/store/i3511kdvz1mk918psg6isvcfz0mng4kf-tmp-project2-exe-tmp-project2-0.1.0.0-ghc-8.10.7-env.drv": [
+ "out"
+ ],
"/nix/store/l5z5blrzvmkkxzah8j9mmhhdal3ppywh-stdenv-linux.drv": [
"out"
],
+ "/nix/store/lxvgibbmjfxr0kmmzw89vg6pr7gvx7r6-foldl-lib-foldl-1.4.12.drv": [
+ "out"
+ ],
"/nix/store/qjlndz62lyrb1xwfk5lysadsfsaqgwx6-zlib-lib-zlib-0.6.3.0.drv": [
"out"
],
- "/nix/store/ykj2l9y4g4hj3ncpd3hc4pdqpn5bc496-tmp-project2-exe-tmp-project2-0.1.0.0-ghc-8.10.7-env.drv": [
+ "/nix/store/v8ccikkvdinwv21z809zyqqwks08l79g-tmp-project2-exe-tmp-project2-0.1.0.0-config.drv": [
"out"
]
},
... |
Notice that we cannot "just" point cabal to a In this way we are free to do The shell script that wraps curl does feel a bit hacky yes. I think the solution based on a local unix-socket accessible webserver,is cleaner https://gist.github.com/andreabedini/d726c191fd7b6eb93dd954115d679547 |
A couple of questions:
Where I'm going with this is: what if we added an overriding repository stanza for each repository that pointed to a |
It turns out they all get appeneded, despite having the same identifier. Clearly this is a code path that nobody has exercised before.
You can
The result is that the plan will still have |
Continuing my dumb questions: why is this a problem at all? |
Because we don't want the plan to depend on how its built. The nix version of the plan should be entirely executable on its own. E.g. it should say: plutus-ledger-api from https:///... at hash 11223344 depends on lens-5.0.1 from https://hackage.haskell.org/package/lens-5.2/lens-5.2.tar.gz at hash xxxyyyzzz with this particular cabal file, etc, etc. |
... why? The plan is there to be consumed by more Nix code? I guess there's a problem if it depends on stuff in the Nix store that might not be there (e.g. if you've materialised it)? Anyway, so trying to again phrase the problem:
|
More dumb questions:
Can we just... not rewrite the repository URLs at this stage? Why are we doing that? Could we let cabal generate the plan using the normal repository URLs, and then just mess with it later when we're building? 🤔 |
One fundamental thing is that changing the plan should not arbitrarily rebuild everything (which would happen in every package derivation depends on the plan). We already do this, it's one of the main features of haskell.nix.
I find it cleaner if the produced plan was much self-contained as possible, but I have to agree it is not an hard requirement. I think we could stll have a plan that points to nix paths for the packages. @hamishmack @angerman what do you think? |
CC @L-as @brainrake |
I'm not much a fan of the |
🤔 likely, I will make sure that will keep working.
I know! but I wouldn't know what exactly we need to fix. The problem is that cabal needs to "initialise the repo" and there's no other way that doing |
We just depend on being able to pass hackages to cabalProject' |
@L-as could you migrate to using something like CHaP instead? |
I went to remove the curl hack as discussed on slack and I realised it's really not necesary at all. The reason we get
is because we leave that url in What I changed is basically to bootstrap hackage just like we do with extra-repositories, which is 1) download the tarball 2) make a temporary Also, I did stress test that the builder derviations do not depend on the plan itself. I kept changing the plan derivation (to add comments, simplify scripts etc) and doing edit: I should have put back all the extra-hackages stuff. |
67b710e
to
1eadeb7
Compare
Cabal2nix? |
Right, I am referring to |
Where are we with this? |
We were waiting for hadrian support to be merged, which has just happened. Now I am updating the materialisations to check that everything is ok. |
Reporting here, @hamishmack has updated all the materialisations but we discovered a bug. In a situation where a plan contains different versions of the same package (e.g. cabal building cabal), haskell.nix somehow applies the cabal file revision from hackage to the local package, which is hell wrong. This prompted me to fix a design wart that should eradicate this kind of issues. I know we are all keen to get this in but it's better to do it right (given the cost of rebuilding everything and busting all the caches). The design wart is that I am passing the cabal file along with their nix version while they should be inside the nix version. Given they are keyed by name, it's too easy to mix them up. |
Cabal files are now obtained from the index tarball (as cabal does) directly. This allows us to always pick the cabal file revision that cabal would pick, without having to understand or reference hackage.
7da4ee8
to
5318f46
Compare
Nix derivations are already able to pass a attribute to the builder as a file. This means we don't need to use pkgs.writeText to turn the cabalFile attribute into a file, saving one derivation.
Hello,
this PR consolidates my effors of the past few weeks. It solves the problem of supporting revisions (#1675) and streamlines the creation of the nix plan.
The major changes are to
call-cabal-project-to-nix.nix
.I replace
cabal v2-freeze
with another tool (./nix-tools/make-install-plan
) thatI changed plan-to-nix to reference the cabal files and their nix version into the nix plan (when available). When we don't have a cabal file in the plan (I assume this happens only for the pre-existing packages) we fall back to old reference to hackage.nix.
e.g. the nix plan for a simple package that depends on zlib looks like this
./cabal-files/zlib.cabal is the exact revision as used by cabal for the plan (according to index-state for example).
./cabal-files/zlib.nix is generated from the same cabal file
There's also another change I had to make around how we pass repositories to cabal. I realised that since we rewrite the repository urls, the install plan will have the wrong pkg-src:
Rather than working around the issue (e.g. by another bunch of substitutions), I found a way to let cabal fetch the repositories without without messing up the urls. Basically I made a fake version of curl that maps urls to paths in the nix store. E.g.
This implementation is quite crude: a shell wrapper that does a simple substitution on the curl arguments, turning, e.g.,
http://hackage.haskell.org
intofile:///nix/store/...
. I have another implementation which uses an actual webserver. Another alternative would be to wrap wget instead which has a much simpler command line interface.This involved quite some changes and as it stands this gets rid of
dotCabal
entirely. (extra-hackages
is just a collateral victim, I could be restored, maybe).Now the plan has the right information
and so that the nix version can have the right source too
I like this approach because it allows us to just do
cabal update
without having to parse and rewrite anything. Extra repositories and index-states are naturally managed by cabal. We only need to provide an input map (which we do already).Last bit was rerouting the cabal file all the way down to the builder, sometimes replacing the revision.
The result is that this works:tm: I checked I can build cardano-node with it.
Final notes:
make-install-plan
andplan-to-nix
should be merged into a single program. As it stands the former converts the plan to json and latter parses the json back again.index-state-hashes.nix
) and the other for cabal's pre-existing packages.I am keen on receiving feedback. Let me know what you think of this.