-
Notifications
You must be signed in to change notification settings - Fork 6
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
Upgrade CICD #12
Upgrade CICD #12
Conversation
Thanks, I will take a look at that! Out of interest, what project are you using ProFound and/or magicaxis for? Are you an astronomer? |
And reading the change to DESCRIPTION - is the X11 requirement necessary? Trying to decide if I really want to add that... I'm sure I should know, but much of the package has code I wrote over a decade ago now! |
hi @asgr thanks for your questions. I try to give more context here, apologies for the wall of text. I'm at MPI for Extraterrestrial Physics in Germany providing IT infra support for one of our groups. Some (not sure of the number) in our team use your set of libraries, specifically ProFound. I personally do not have a background on astronomy or R, but yes on other software. So I've been learning about R packaging to figure out why is ProFound failing to install in docker containers. Magicaxis is not the issue btw, I'm now just using it as a starting point for me to understand how all clicks together since it has no other asgr dependencies. I can elaborate on my findings in another channel if you prefer (is email ok?). Regarding the change in DESCRIPTION, it is not necessary per se. Let me know if this makes sense, happy to elaborate on the reasoning. One question for you, it seems a lot of your packages need manual installing because the have not made it into the current CRAN repo (ProFound, Rfits, etc are not there), is there a reason for that? if you were able to publish your latest binaries to the official repos then most people wouldn't even need to compile it manually. Again not an expert, but can probably help automate it with a bit of guidance. |
Both Rfits and Rwcs contain lower level C libraries (CFITSIO and wcslib) that generate enough 'warnings' that they never pass the overly zealous CRAN checks. I've tried to justify this to CRAN, but no dice. We have similar (very minor) issue with Cimg (in imager) in the past too, but we made the couple of minor edits ourselves (and I think we even did an upstream pull request). For CFITSIO and wcslib, we've reported this, but to date we've never seen a version creating few enough warnings that we could fix it up and get it on CRAN. |
@keponk the rabbit hole goes even deeper... my turn to say sorry too for the BIG wall of text, I both wanted to document my thinking, and also thought you'd want to know all the details. tl;dr: changes look fine to me except the added X11 dependency, which is covering for a deeper issue. First, thanks a lot for this effort! The current CI workflow has clearly aged, and it can clearly be handled in a much better way nowadays. I agree with @asgr's comment about adding the If you have a look at the latest failure in your GHAs you'll see that the issue is not with magicaxis, but rather with Given that imager issue, and the discussion on this PR, it's clear to me now that the real underlying problem is that an With the actual error clear, here's how I think it actually did work for you:
To demonstrate this I pushed one commit on top of your changes to disable the cache entirely (showing what would happen here). You can see the associated action fail. After all of that:
|
thanks @asgr and @rtobar for the thorough explanation. This does help me understand the workflow (and R world in general) better. As extra curiosity, when I look at the info here: https://p3m.dev/client/#/repos/cran/packages/overview?search=imager&distribution=ubuntu-24.04 Finally, I'm just trying to figure out, what is the "ideal world" scenario here. Is it to be able to publish the binaries to CRAN? And if CRAN rules are too strict, maybe a more detailed README.md or a Dockerfile that successfully compiles the project, could be nice alternative that speeds up usage. I would have some bandwith to help with the latter. |
With the changes from |
Thanks all - happily passed the CI, and just merged :-D |
I've been following down the rabbithole of these packages from ProFound build. Arrived here as it is one of your packages that is not dependent on another of your custom packages (similar to imager).
Using
asgr/imager
repo as a guide, as well as r-lib template, I have upgraded and cleaned up this github workflow. In summary:setup-r-dependencies
to manage required ubuntu dependenciescheck-r-package
action