dockerTools.buildLayeredImage: store all paths passed in final layer #78786
Conversation
|
Can you add a test to the examples list which forces the last layer to have many, and runs a program which would appear in the "bulk" layer? This way we can test it more easily next time. |
Fixes NixOS#78744 My previous change broke when there are more packages than the maximum number of layers. I had assumed that the `store-path-to-layer.sh` was only ever passed a single store path, but that is not the case if there are multiple packages going into the final layer. To fix this, we loop through the paths going into the final layer, appending them to the tar file and making sure they end up at the right path.
purefn
force-pushed
the
fix-including-more-than-max-layers
branch
from
a31daa3
to
0cf37e5
Compare
|
@grahamc Great idea! Done. |
|
@purefn Thanks for you quick fix! If we want to quickly fix this issue, we could only merge the fix (without the test) and I could work on a test tomorrow, once #78834 is merged. |
|
If we want to avoid that bug, why not package |
How is it not working as expected? It builds an image with 3 layers: the first is the layer with That's exactly what I expected it to produce when I wrote the test. Were your expectations different? |
|
@purefn Sorry, I should have provided more explanations ;) In the image you are building, the second layer contains both To easily write a test, I think we should write a test loading an image containing only 2 layers: the "bulk" and customization layers. In the bulk layer, we could add several store paths and ensure all of them are in the image. This test is more complicated to write with several layers because it hard to know the destination layer of store paths. |
|
@purefn I was thinking on a test such as in nlewo@bad4290 |
|
I'm not opposed to your test, I think it's fine. I'm just curious, wouldn't we get effectively the same result if we changed the docker image command to something along the lines of |
|
Yes, you are right, Actually, I was initially thinking on just creating these two files (because they don't have any dependencies)... but |
|
@purefn It would be nice to merge your fix ;) |
|
Oh, sorry! I've been swamped with work and life the past few weeks. |
Motivation for this change
Fixes #78744
My previous change broke when there are more packages than the maximum
number of layers. I had assumed that the
store-path-to-layer.shwasonly ever passed a single store path, but that is not the case if
there are multiple packages going into the final layer. To fix this, we
loop through the paths going into the final layer, appending them to the
tar file and making sure they end up at the right path.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)