Fix Python tests #236
Fix Python tests #236
Conversation
scripts/mk_repo_file.py
Outdated
| "--repo-url=https://github.com/danielfullmer/tools_repo", | ||
| "--repo-rev=9ecb9713ee5adba95120acbc0bfef1c77b02637f", | ||
| "--repo-url=https://github.com/jaen/tools_repo", | ||
| "--repo-rev=dca531f6d6e9fdcf00aa9d18f0153bd66a2e32ea", |
There was a problem hiding this comment.
We should probably just turn this into a plain old patch simply apply it to the regular gitRepo drv.
There was a problem hiding this comment.
That sounds reasonable,I'll try implementing it like this
There was a problem hiding this comment.
Turns out that doesn't help much, because the regular gitRepo derivation is basically just a launcher (c.f. https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/version-management/git-repo/default.nix#L32) that will then download the missing code from the interwebs (c.f. https://github.com/jaen/tools_repo/blob/main/repo#L669-L674). While it would be nice to make it reproducible, I'd say it's maybe out of scope for a simple test fix?
There was a problem hiding this comment.
Did overlay v2.45 in pkgs, though. Not sure if there's any downside to those versions not being the same, but might as well have them match.
| freetype # Needed by jdk9 prebuilt | ||
| fontconfig | ||
|
|
||
| # Goldfish doesn't need py2 anymore in Android 12+! |
There was a problem hiding this comment.
Okay not 100% sure how to trace this commit to Android version, but I think that's right
|
FWIW I have successfully built the following ROM on this branch: Can't test it, because it's not my phone, but at least something builds. |
|
Please note that the files you touched don't really influence build results very much but rather generating the sources. |
|
@Atemu I would think I have also just now modified the I ran update scripts for GrapheneOS (used |
|
Re: above — added |
jaen
force-pushed
the
fix-python-tests
branch
4 times, most recently
from
5991c71 to
c21880a
Compare
scripts/robotnix_common.py
Outdated
| prefix = os.getenv('NIX_REMOTE'); | ||
| prefix = os.getenv('NIX_REMOTE') | ||
| if prefix and not prefix.startswith('/'): | ||
| raise Exception('Must be run on a local Nix store.') | ||
| return f"{prefix}/{path}" | ||
| return os.path.join(*filter(None, [prefix, path])) |
There was a problem hiding this comment.
Sorry about that.
I'd prefer @eyJhb's patch from here: https://github.com/nix-community/robotnix/pull/228/files#diff-d2cdd059d29643c5fe1767c6dcf5203c0dc9b382d1322e3f0a4237eda6066246 though.
There was a problem hiding this comment.
Hmm, can you explain why? I've done it like this, so this function doesn't inadvertently turn a relative path to an absolute one rooted at /. Is doing that a desired behaviour of this function?
There was a problem hiding this comment.
This function's purpose is to turn a nix store path to an absolute path that can be opened on the filesystem.
I've rewritten it, could you take a look?
There was a problem hiding this comment.
Seems reasonable; I'll re-test it locally just in case.
There was a problem hiding this comment.
Ok, after testing I'm kind of confused on what exactly NIX_REMOTE is. Nix itself seems to have that parameter (https://nix.dev/manual/nix/2.22/command-ref/env-common#env-NIX_REMOTE) but it doesn't seem to be that (because it's supposed to be a daemon socket).
From the context it looks like a mountpoint for some alternate store — but also not quite. When I added some logging to the code I get paths like /nix/store/deadbeef-hudson, and if you relativise them to root, you'll end up with paths like /your/prefix/nix/store/deadbeef-hudson which makes it not a store path IMO, I'd rather expect /your/prefix/deadbeef-hudson in this case, as if you specified NIX_STORE_DIR (https://nix.dev/manual/nix/2.22/command-ref/env-common#env-NIX_STORE_DIR) with that value.
But this is a bit of a documentation issue, I guess. I proposed a bit improved implementation in a separate commit — it leaves the path untouched if NIX_REMOTE is not set, otherwise it does the same logic but with pathlib which makes it a bit cleaner, as you don't have to worry about . not being resolved or knowing what is the root path to relativise the store path against.
There was a problem hiding this comment.
NIX_REMOTE specifies a "remote" nix store. It's the equivalent of the --store flag.
If you set it to a local path, nix would then use the nix store under that prefix but it does not change the NIX_STORE_DIR. Build sandboxes are mounted such that the paths are available under /nix/.
You can't directly run software off of such a store without a chroot but that's of little relevance for my robotnix builds.
The reason I use this is to store the 60GB+ build-time closure and hundreds of prefetched source dirs in a separate Nix store to my system's because I don't want to trash it. I explicitly do not want the NIX_STORE_DIR to change here.
(You can also point it at another machine via ssh in which case it builds on the remote machine but that doesn't work for the updater.)
In the updater, we need to be able to access the lineage.dependencies file in a fetched repo and to do that while NIX_REMOTE points at some other location on the filesystem, you need this function. The returned string is not a store path but an absolute path that you can simply open and read.
Thanks for the pathlib patch, that looks a lot more robust. I don't have any experience with python to speak of, so this is much appreciated.
There was a problem hiding this comment.
Yeah, I did roughly understand what this function is for and it's a reasonable thing to want — I just wasn't sure what exactly it represents in terms of nix config/options/envvars (since nothing seemed to naively match it) and as such what I should expect the correct behaviour to be when testing it.
But it makes a lot more sense now that you explained what it's supposed to represent in terms of nix options/behaviour and I guess it actually indeed is NIX_REMOTE as described here, it's just that the description in the manual doesn't really make it clear that all values from this list are valid and your explanation clarified that.
|
I'm currently cleaning this up a little and will force push your branch in a bit, just FYI. |
Atemu
force-pushed
the
fix-python-tests
branch
2 times, most recently
from
0926f07 to
adf0c0e
Compare
It wouldn't work without NIX_REMOTE set before. This is a lot more robust now.
Suggested-by: Atemu <[email protected]> Reviewed-by: Atemu <[email protected]>
The `repo` tool derivation by default includes only the main wrapper script which fetches the actual tool sources from the internet. We modify the derivation to provide default local sources patched with support for repo2nix, unless specified otherwise with CLI parameters. Flake compat was updated and nixpkgs-unstable re-introduced to facilitate this change.
Makes
pytest .ran fromnix developpass.Fixing the
formatterissue mentioned in #235 required updating therepotool to the newest version. Sincemk_repo_fileuses a fork that adds additional commands to dump repo information, I have rebased it on top of the upstream v2.45 (see jaen/tools_repo#1 for diff). Not sure where do we want to put this, though.