-
Notifications
You must be signed in to change notification settings - Fork 16
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
Handle GitHub tag pr #420
Handle GitHub tag pr #420
Conversation
Okay maybe I was a little to optimistic. If i run
which is
I get
Actually the building step works fine and I can activate my shell and then get rix v0.8.0 as requested. However, I am little confused by this error message. Does that mean that GithubAPI is then used during the building process? This is probably not want we want? And why does this succeed despite the warning? @b-rodrigues |
Okay, I changed this now because I think what I really wanted to test (similarly as with the PR) is not a tag given by the user, but a tag given in a description file. I could not fine one easily, so I created a test github repo. I tested that the resulting default.nix build successully and has rix version 0.8.0.
@b-rodrigues I think this is more what we are looking for, right? Although I found it interesting that specifying a tag instead of a commit in git_pkgs worked despite this warning. And I am a little confused because the rix part is actually exactly the same, but I don't get that API request warning anymore or is this because it's using caching? |
Hm this tag thing is more complicated than I thought. I think the big difference is that before I was resolving the package dependencies in R and tried to get a commit, which has a close date and then nix works fine. Here, I thought that it's fine to just use the tag. I though this because that's what you did with your strsplit function (tried to get the ref) and also using try_download with a tag instead of has works fine
However, I now realize that try_download working is only one part (to get the correct imports), but then also the resulting default.nix has to work with nix (and this is not tested with the tests in nix). As I said in my previous post that worked fine on my local machine. However, to be on the safe side I tried to run the same rix call in a Dockerfille, so generate_env.R
Don't think i have seen these thousands In the end this build is successful, and I also get the expected version. However I also get these strange warnings agains However, I then changed generate_env.R to something without tag and without remote package, but still got the same warnings
So i think this is not related but a different issue that should be addressed separately? So to sum up @b-rodrigues
|
The way this is done here is through dedicated actions like this one: https://github.com/ropensci/rix/blob/main/.github/workflows/test-renv2nix.yaml An expression gets generated, and then built. If it fails, we know the generated expression is wrong on some level. This actually really helped me yesterday, when I was working on the other PR. Could you add an action like this (with as many steps as needed to build as many default.nix files as needed) to this PR?
I’ve seen these as well, gotta admit I have no idea what it is.
|
Thanks, that's exactly what I meant. I started to setup a github workflow for PR, but I don't think we need that. Because since rix now removes the PR, it will use download_all_commit and then get the commit hash, and we don't need to test that this works. rix/tests/testthat/test-fetchers.R Line 137 in 0aed1a0
and this seems to work fine :)
|
https://github.com/ropensci/rix/actions/runs/13260868909/job/37016963753 |
Probably an update to the action install nix, setting up some option or something that doesn't make sense on GA? |
So using a tag name, rix can just pass it to nix and this works fine
but if using a branch name like this, it does not work.
The problem is that both are separated by a So I think i will just not only remove everything and including |
ah I forgot, we may want to keep it if it's a commit hash. However this should be doable with a regex. |
@b-rodrigues So i think I am done. The way this is handled now. Ignore PR (so Looking forward to your revision |
Ah one last thing. In github workflow I had to use my fork for now, because it's not merged. After you merge this I think it's better if you would change the url, so this line in test-fetch-commit-short.yaml
|
"\n rix = (pkgs.rPackages.buildRPackage {\n", | ||
" name = \"rix\";\n", | ||
" src = pkgs.fetchgit {\n", | ||
" url = \"https://github.com/ropensci/rix\";\n", |
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.
So if I understand correctly, rixTest has Remotes: ropensci/rix#100
in its DESCRIPTION, but the PR simply gets ignored. However, the commit that gets used is then the closest to the one from rixTest, right?
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.
That’s how it’s described in the vignette, so I guess that’s right
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.
yes. PR, branch and tags are ignored (although tags would work with the nix expression, but cannot be differentaited from branches I think), only commit shas are used. I think this makes sense and nicely fits to rix philosophy of always trying to use a commit hash. So if a commit hash is given it's used, all others fixed tags/pr/branch are not used.
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.
and yes if nothing is speciied then the resolve_package_commit is used to fetch the closest commit hash.
I just tried to rerun this one:
and got :
shouldn't this be ignored now? |
Sorry, cannot reproduce this. Using the main branch, your rix calls runs successfully
|
However, the default nix still has the duplicate, which you aimed to remove with your postprocessing that is already merged to main branch I think.
|
@b-rodrigues tries to address some of the problems learned from #416
If there is a remote dependency with a PR (so e.g.
pkg#27
) this will be removed (so it will then call resolve_package_dependencies to get the closeset commit)Tags work out of the box.
Added tests for those situations.