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

Grab proot from bootstrap zip rather than including its nix path #393

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shelvacu
Copy link

This means that the cachix substituter (or already having the package in your nix store somehow) is no longer required to build.

This required reworking the deploy script. As a bonus you can now omit the second argument and it will tell you what it would have copied instead of copying anything.

This is fixes one source of impurity, but for now flake builds will still require the --impure flag

@@ -0,0 +1,5 @@
# WARNING: This file is autogenerated by the deploy script. Any changes will be overridden
{
url = "file:///data/local/tmp/n-o-d/bootstrap-aarch64.zip";
Copy link
Collaborator

@t184256 t184256 Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume, the committed version will have my bootstrap url here instead?

Isn't there a dependency loop, BTW, with the bootstrap zipball containing the hash of itself?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"will have my bootstrap url here instead" yes

"isn't there a dependency loop"
No, two things:

  • the hash is only a hash of the proot binary
  • during bootstrap building prootStatic is overridden, so this is never evaluated

@shelvacu
Copy link
Author

updated to fix formatting

cat >$new_attrs_file <<EOF
# WARNING: This file is autogenerated by the deploy script. Any changes will be overridden
{
url = "$NIX_ON_DROID_BASE_URL/bootstrap-$arch.zip";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lacks a branch (github:owner/repo/branch case, see above)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, wait, it generally isn't inferrable. The interface will have to be extended to pass the public url of the zip tarball as well =(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case I want covered is, basically, someuser cloning nix-on-droid, creating a branch, changing it and then wanting to test it without adb and with a HTTP server. while we know where to pull the github source tarball from if it's on github (and non-github users assumed to know what they're doing), we don't know what URL will they place the bootstrap tarball at. previously that didn't matter to us, as long as they enter it correctly into the app; now it begins to matter

cat $attrs_file
echo ">>>>>>"
log "adding $attrs_file to git index"
git add $attrs_file
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to, otherwise it's not included in the flake. I agree it's less than ideal but I don't see an obvious alternative.

@t184256
Copy link
Collaborator

t184256 commented Aug 7, 2024

This is fixes one source of impurity, but for now flake builds will still require the --impure flag

And what's the other impurity source?

@shelvacu
Copy link
Author

shelvacu commented Aug 7, 2024

This is fixes one source of impurity, but for now flake builds will still require the --impure flag

And what's the other impurity source?

  • Reading from env vars
  • uid/gid
    There may be others but those are the two I'm aware of

@shelvacu shelvacu force-pushed the shelvacu-purify branch 3 times, most recently from 082fe84 to a18149c Compare August 28, 2024 03:41
@t184256
Copy link
Collaborator

t184256 commented Aug 29, 2024

I've made one reading pass through it, I am willing to explore that direction in general, but I'm afraid I'll make a million little opinionated edits over the course of the next ~10 days.

@shelvacu
Copy link
Author

Opinionated edits aren't necesarily bad, I believe I checked the box so that you can directly push changes to the branch if you want to.

@t184256
Copy link
Collaborator

t184256 commented Aug 29, 2024

So far the only global consideration I've found for my workflows is that I can't update proot in between releases, lest we'll have master commits using a version of proot with no stable-path zipball to back them.

@shelvacu
Copy link
Author

shelvacu commented Sep 2, 2024

So far the only global consideration I've found for my workflows is that I can't update proot in between releases, lest we'll have master commits using a version of proot with no stable-path zipball to back them.

I don't know what you want from me on this one. If you do it anyways it's no worse off than before (sometimes people will need to have the cachix substituter), or you can follow a nightly-style filename format (eg /bootstrap-unstable-2024-09-01/bootstrap-aarch64.zip, would also take up more space ofc), or you switch to a hash-only (no url) fixed-output derivation.

…ctly.

This means that the cachix substituter (or already having the package in your nix store somehow) is no longer required to build.

This required reworking the deploy script. As a bonus you can now omit the second argument and it will tell you what it would have copied instead of copying anything.

This is fixes one source of impurity, but for now flake builds will still require the --impure flag
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