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

Upgrade CICD #12

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Upgrade CICD #12

merged 3 commits into from
Oct 18, 2024

Conversation

keponk
Copy link
Contributor

@keponk keponk commented Oct 14, 2024

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:

  • Bumped up versions of different actions
  • As suggested by the r-lib actions group, setup pandoc earlier
  • Stick to Ubuntu LTS releases
  • Let brew manage xquartz installation
  • SystemRequirements field allows setup-r-dependencies to manage required ubuntu dependencies
  • Cache and upload are already managed by check-r-package action
  • Remove gitignore from github workflow folder and add R gitignore

@asgr
Copy link
Owner

asgr commented Oct 15, 2024

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?

@asgr
Copy link
Owner

asgr commented Oct 15, 2024

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!

@keponk
Copy link
Contributor Author

keponk commented Oct 15, 2024

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.
But the reasoning here is to "help" the github workflow setup-r-dependencies action (see here ). As that action looks at the DESCRIPTION file to make decisions on what to install as dependencies, both module deps and system dependencies (see here ). So by adding that one line, you get to remove the whole section of "system dependencies" you had on the github workflow file. Moreover, IIUC, this will help any other R package manager figure out what it needs to fulfill the compilation of that module (be it command line, rstudio, etc).

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.

@asgr
Copy link
Owner

asgr commented Oct 16, 2024

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.

@rtobar
Copy link

rtobar commented Oct 16, 2024

@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 SystemRequirements on X11. magicaxis doesn't use X11 itself though, so it doesn't make sense to state the dependency. OTOH you mentioned that you added it to help the setup-r-dependencies step to install the correct set of dependencies. I believe this is actually a fairly convoluted red herring, and if @asgr approves and runs the workflow that is pending in this MR, I'm fairly certain it will actually fail.

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 imager (which we also maintain under asgr/imager). It is the compilation of this package that requires the X11 headers and libraries. However, X11 is already listed as a system dependency of imager. This means that the calculation of the system packages from the DESCRIPTION file isn't working as expected. See, again in your latest failed build, how X11 is listed in sysreqs, but there are not associated packages listed below. Moreover, it means that adding X11 to magicaxis won't fix things. We actually had someone report a similar issue in imager a few days ago, where their build stopped working overnight due to missing X11 headers/libs for imager. Tuns out these development headers/libs apparently came pre-installed in ubuntu-22.04, but not in 24.04, which is what ubuntu-latest (unexpectedly to him) now points to.

Given that imager issue, and the discussion on this PR, it's clear to me now that the real underlying problem is that an X11 value in SystemDepedencies isn't translating to the correct system packages. Going down from actions/setup-r-dependencies, this translation is internally handled by r-system-requirements. Since you mentioned you've just started to delve into R packaging, you might not be aware that SystemRequirements is pretty much a free-text field to list system-level dependencies. r-system-requirements uses a set of rules to regexp the SystemRequirements and determine packages for different OSs and package managers. However, there's no rule for X11. Given how it's already bitten people twice in a week, I'll be opening a PR to provide that support now. pak, the tool used by actions/setup-r-dependencies should then use the new rule automatically after a day or so.

With the actual error clear, here's how I think it actually did work for you:

  • In your first successful workflow build a step installed xvfb, which included the installation of libc11-dev
  • The build succeeded and your package cache got populated
  • Successive workflow executions skipped the package installation as it was already in the cache. So even though by then you had already removed the xvfb instlalation, and the X11 headers/libs were not installed anymore, you didn't experience any problems.

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:

  • Let's not add a SystemRequirements to magicaxis
  • I'll create the PR against r-system-requirements and see how that goes, I'll report back
  • If you're eager to go ahead in the meanwhile, either add a job in the workflow to explicitly install libx11-dev early on, with a comment as to why this is needed
    • Alternatively, switch to using ubuntu-22.04, which apparently came with libx11-dev pre-installed

@keponk
Copy link
Contributor Author

keponk commented Oct 16, 2024

thanks @asgr and @rtobar for the thorough explanation. This does help me understand the workflow (and R world in general) better.
All assumptions from @rtobar are correct, I confirm that after deleting the cache from my own workflow builds it failed and so the X11 still needs to be manually installed. I also see the contribution @rtobar has made to the r-system-requirements group. IIUC, once his contribution is merged, the action should then correctly cover the dependencies from imager (and any other package stating X11 as a SYSREQ) and install the X11 libs , leaving this PR with only the changes on the workflow (and not the DESC file). I've updated this PR accordingly and happy wait and keep an eye until the changes on r-system-requirements have gone into effect.

As extra curiosity, when I look at the info here: https://p3m.dev/client/#/repos/cran/packages/overview?search=imager&distribution=ubuntu-24.04
I also see the dependency on X11, and this website goes also as far as showing you the command necessary to install them. But as you can see, although X11 is listed, the ubuntu packages required are not. For now I would assume they are also looking at the same public DB from r-system-requirements and maybe that website will soon update accordingly. If not, I'd be curious if there are other DBs to keep an eye on.

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.

@keponk
Copy link
Contributor Author

keponk commented Oct 17, 2024

With the changes from r-system-requirements indeed the action now takes correct care of dependencies. I also added a template R gitignore and removed the one form github workflow as it didn't seem to have a relevant effect on the repo.

@asgr asgr merged commit d5c119b into asgr:master Oct 18, 2024
3 checks passed
@asgr
Copy link
Owner

asgr commented Oct 18, 2024

Thanks all - happily passed the CI, and just merged :-D

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.

3 participants