Skip to content

Conversation

mihem
Copy link
Contributor

@mihem mihem commented Apr 18, 2025

@b-rodrigues as discussed here #467
rix should work with github/gitlab packages that use a subdirectory to store the R package like BPCells.

@mihem
Copy link
Contributor Author

mihem commented Apr 18, 2025

Ah I did not pay attention to the fact that hash_url is not only used by github packages but also by cran packages, which don't have a repo_url or commit

@mihem
Copy link
Contributor Author

mihem commented Apr 18, 2025

well actually it works with cran packages but not with archive packages because the they use hash_url, try to fix this

@mihem
Copy link
Contributor Author

mihem commented Apr 18, 2025

So I think I am done with the PR, all tests pass.
@b-rodrigues when you have time, please take a look

I think i will add two sentenced in the docs, will have time again on Sunday

R/nix_hash.R Outdated
url
)
commit <- gsub(x = basename(url), pattern = ".tar.gz", replacement = "")
commit_date <- get_commit_date(repo_url_short, commit)
Copy link
Contributor

Choose a reason for hiding this comment

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

just realized this is only being done for github: maybe in another PR this could be extended to gitlab as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, although this is more complicated than changing it here, because this relies on API github, github token etc, so yes maybe something for a future PR although I think we have seen several users relying on github remotes and not any relying on gitlab, also don't have any package from gitlab i use.

@mihem
Copy link
Contributor Author

mihem commented Apr 20, 2025

@b-rodrigues I've created a function to generate the regex (with some help by Claude Sonnet), should be less redundant now, also created some docs, hope you are happy with that.

@mihem
Copy link
Contributor Author

mihem commented Apr 20, 2025

btw found naother prominent example mentioned on the remotes github site.
https://github.com/dmlc/xgboost

@b-rodrigues
Copy link
Contributor

btw found naother prominent example mentioned on the remotes github site. https://github.com/dmlc/xgboost

xgboost is on cran, so it's available through nixpkgs as well

@mihem
Copy link
Contributor Author

mihem commented Apr 21, 2025

btw found naother prominent example mentioned on the remotes github site. https://github.com/dmlc/xgboost

xgboost is on cran, so it's available through nixpkgs as well

Yeah sure, but if you want to install the dev version (as mentioned on remotes here https://github.com/r-lib/remotes) you need a package which supports subfolder, which rix currently does not

@mihem mihem requested a review from b-rodrigues April 21, 2025 15:46
@mihem
Copy link
Contributor Author

mihem commented Apr 23, 2025

@b-rodrigues okay i put it in the first if statement, was some additional work and code.
I think it's cleaner now, and everything works fine. There is only on tests that fails regarding the lookup package from your repo.
I just corrected the output because I thought just the order was different but i now saw that i no longer has memoise and gh as remote output, need to investigate further why this is.

@b-rodrigues
Copy link
Contributor

you should revert that last commit, as the test is not fixed

@mihem
Copy link
Contributor Author

mihem commented Apr 23, 2025

yeah I know, I try to understand why .. not sure yet, do you see it?

@mihem
Copy link
Contributor Author

mihem commented Apr 23, 2025

ah I have a clue
Error: Directory/tmp/RtmpqYCzaH_repo_hash_url_iypan/file335954f596fa1/package_src/gh-HEADdoes not exist

I use the commit now, previously we downloaded and just used the existing directory. THis usually works fine, but not if the commit is head. The correct dir name is
gh-56692f828c2d20cfacf5682a10d77fcca59e41dc
hmm, not sure if we can easily get this

mihem added 2 commits April 23, 2025 20:26
This reverts commit 9107909.
this previously failed because it relied on commits
however if the commit was not fetched the dir path would be wrong.
Therefore, this now uses again two if statements, or before try download
and one after download, to get the correct path via `list.files()`
after download.
@mihem
Copy link
Contributor Author

mihem commented Apr 23, 2025

@b-rodrigues so the previous attempt of using one large if statement failed because this relied on having a commit to get the correct r path instead of relying on the file path to the repo after it had been downloaded.
This mostly worked, but sometimes failed if fetching the commit failed and it fell back to HEAD.

Since this could also happen, if people don't use a github token or so, I don't think this is a robust approach.
I therefore went back to the previous approach: one if statement before downloading to get the root_url (without subdir).
And then downloading the package.
And after that using the path to the package to build the path to the subfolder.

I simplified the if statement by just setting root_url and path_to_r and then adjusting it if Github/gitlab + subdir.
Hope we can finally finish this then.

@mihem mihem requested a review from b-rodrigues April 23, 2025 20:18
@b-rodrigues b-rodrigues merged commit a99b262 into ropensci:main Apr 24, 2025
26 of 27 checks passed
@b-rodrigues
Copy link
Contributor

Many thanks!

@mihem
Copy link
Contributor Author

mihem commented Apr 24, 2025

Welcome

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