conda-forge / r-jpeg-feedstock

A conda-smithy repository for r-jpeg.
BSD 3-Clause "New" or "Revised" License
0 stars 6 forks source link

rebuild against latest `r-base`, to properly link against `libjpeg` provided by requirements of `r-base` #12

Closed dlaehnemann closed 1 year ago

dlaehnemann commented 1 year ago

Checklist

r-base has recently switched from using the jpeg-feedstock to using the libjpeg-turbo-feedstock. And r-jpeg pulls in r-base and all its requirements:. But the recipe here currently just links against any libjpeg it finds, which it seems can be a system libjpeg with version libjpeg.so.9 coming from the Independent JPEG Group. This is incompatible with the libjpeg.so.8 that r-base pulls in from libjpeg-turbo. I have just encountered this in an error while building and using the dada2 bioconda package:

unable to load shared object '/tmp/tmpww2pfh35/test/.snakemake/conda/e47cfa7b279a93ef7fb06d189e043ebb_/lib/R/library/jpeg/libs/jpeg.so':
  libjpeg.so.9: cannot open shared object file: No such file or directory

I am hoping that these changes fix that and keep the recipe future-proof for any further switches of the respective library.

conda-forge-webservices[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

dlaehnemann commented 1 year ago

@conda-forge-admin, please rerender

dlaehnemann commented 1 year ago

Hmm, this does not seem to do what I thought it would. I'll investigate further on the other end that I mentioned above, what the exact issue is.

dlaehnemann commented 1 year ago

Maybe, this package just needs to be rebuilt against the latest r-base to fix my above problem. So I am trying this, now.

mfansler commented 1 year ago

If this links against libjpeg then it should be in the host requirements. That way, coordination is achieved through the conda-forge-pinning-feedstock.

mfansler commented 1 year ago

Also, would you mind filing an issue? We should patch the metadata on the previous builds to explicitly declare the libjpeg versions they linked against.

mfansler commented 1 year ago

FWIW, I found a similar omission with the r-tiff recipe not too long ago. I haven't gotten around to the metadata patching, but you can look at how the recipe was updated.

dlaehnemann commented 1 year ago

If this links against libjpeg then it should be in the host requirements. That way, coordination is achieved through the conda-forge-pinning-feedstock.

I think it should now link against the libjpeg provided by libjpeg_turbo (sorry, I had originally omitted the -turbo in one instance, above), but am not sure that we want to explicitly put this dependency anywhere in the meta.yaml here. This recipe depends on r-base and depending on which version of r-base it is built with, we just want to pull in whatever r-base already depends on. And I think the discrepancy I saw merely came from this recipe not being re-rendered upon the update of r-base from jpeg to jpeg-turbo. And not specifying this explicitly seems more robust to me, or would the conda-forge tooling pick up such a migration in the future, and update and re-render the recipe, if we explicitly specify the dependency here, as well?

Sorry, I am new to these intricacies of dependencies, so I don't fully understand what's going on...

mfansler commented 1 year ago

Hmm. The libjpeg-turbo complicates matters because the migrations for 4.1 are not building. While letting it use what is in the host environment works, it creates a cryptic dependency. Ideally, the solver should know explicitly about runtime requirements via the metadata. So as a rule of thumb, any libraries linked in the Makevars of an R package should be declared in the meta.yaml.

My thinking is that unless this is urgent, we wait for someone to also get the 4.1 migrated, then explicitly declare the dependency. Meanwhile, patching the metadata seems most urgent, so that the solver knows older versions are incompatible with the latest R w/ libjpeg-turbo builds.

Otherwise, we temporarily use selectors build variants to depend on jpeg in 4.1 and libjpeg-turbo in 4.2.

dlaehnemann commented 1 year ago

From what I can see from the tests right here, we currently have r-jpeg successfully building against jpeg in 4.1 and successfully building against libjpeg-turbo in 4.2:

jpeg:                      9e-h0b41bf4_3              conda-forge
r-base:                    4.1.3-h2f963a2_5           conda-forge

and

libjpeg-turbo:             2.1.5.1-h0b41bf4_0         conda-forge
r-base:                    4.2.3-h062c8ea_1           conda-forge

This is true for all the builds in linux_64_r_base, linux_ppc64le_r_base and osx_64_r_base. The only exception is the win_64_ build, which only builds for r-base 4.1 and (successfully) links against libjpeg-turbo:

m2w64-libjpeg-turbo:            1.4.2-3                 conda-forge
r-base:                         4.1.3-hdca333a_5        conda-forge

I do not understand how this happens...

But generally, I think the r-base in host: and run: requirements already does its job, no matter if r-base 4.1 is also available with linking against libjpeg-turbo. r-jpeg should work with either jpeg or libjpeg-turbo, it just needs to be consistent with the r-base it uses and the r-base dependency should ensure this (via the solver). Thus, having no pinning here should be what we want, right? And we don't need to do any further metadata annotation?

Then, we could just merge this here, to upload a new build that links against libjpeg-turbo for r-base 4.2.

But maybe I am just not seeing something, here.

And I did have this case where somehow the linking during build was done against jpeg and then only the libjpeg-turbo version was available for the runtime. So maybe this gets messed up, once other packages have a clear dependency on one of those versions in build:?

mfansler commented 1 year ago

The issue is that if a r-jpeg is built against libjpeg-turbo, the solver needs to know about this in order to know that it can only work with r-base versions that were also built against that. Otherwise, you're setting up end users to hit a bunch of DLL errors, i.e., degrading the user experience.

It really is not about getting the build working here (e.g., lots of autotick PRs pass all tests with latest versions, but still need their metadata update). It is about the solves that end-users perform being reliable.

conda-forge-webservices[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

dlaehnemann commented 1 year ago

Generally, I am sidestepping this in the case where this arose, by not triggering the respective tests. This gets my respective PR merged, but obviously doesn't prevent users from running into this. So I am fine with this waiting for the migration to pass for 4.1, but I guess this should be fixed soon.

As the packages for different versions are named differently, the zip_keys syntax doesn't work. I've read through the conda build docs on build variants in detail, but haven't found a syntax that would work. So the current suggestion is just to document what I think would need to be done. Which is specifying two build variants, where different (partly overlapping) sets of r-base versions are available with different dependencies. But:

  1. I don't know how to put this in correct syntax (maybe a conda_build_config.yaml and selectors could help?)
  2. I am not sure how packages behave after such a migration. For example, once r-base 4.1.3 is available with libjpeg-turbo, is the previous build of r-base 4.1.3 that requires jpeg still available? I am assuming that yes.
dlaehnemann commented 1 year ago

Another way of maybe implementing these dependencies could be the json format described for the --variant flag of the conda build command. But I'm not sure where to put this consistently. It would have to look something like this:

conda build recipe --variants "[ { r-base: 4.1, jpeg: 9 }, { r-base: 4.2, libjpeg-turbo: 2 } ]"

But again, I am not really sure about syntax. The examples in the docs are a bit all over the place and a scenario like this one simply doesn't come up....

mfansler commented 1 year ago

@conda-forge-admin please rerender

mfansler commented 1 year ago

I added the zipped build variants. However, it still needs to avoid the win-64 + R 4.2 variant. Hopefully, this make sense how it works.

I'm also not sure why the linter isn't passing. Seems like it simply didn't rerun after my commits.

mfansler commented 1 year ago

Okay, that looks like it get's all the variants specified without the win+r42 now. Still unclear what's up with the linting failure. I checked offline as suggested and it dies with:

$ conda smithy recipe-lint recipe/.
Traceback (most recent call last):
  File "/Users/mfansler/miniconda3/bin/conda-smithy", line 10, in <module>
    sys.exit(main())
  File "/Users/mfansler/miniconda3/lib/python3.9/site-packages/conda_smithy/cli.py", line 669, in main
    args.subcommand_func(args)
  File "/Users/mfansler/miniconda3/lib/python3.9/site-packages/conda_smithy/cli.py", line 513, in __call__
    lints, hints = lint_recipe.main(
  File "/Users/mfansler/miniconda3/lib/python3.9/site-packages/conda_smithy/lint_recipe.py", line 980, in main
    results, hints = lintify(meta, recipe_dir, conda_forge)
  File "/Users/mfansler/miniconda3/lib/python3.9/site-packages/conda_smithy/lint_recipe.py", line 478, in lintify
    req, _, _ = requirement.partition("#")
AttributeError: 'NoneType' object has no attribute 'partition'

But nothing is popping out at me.

zklaus commented 1 year ago

I used pdb to trace the issue. Looks like the - {{ libjpeg }} jinja lands in Conda-smithy as a None, which triggers the above error. Not sure if the jinja expression is correct and Conda-smithy wrong, or the other way around. From what I see in the libjpeg-turbo migration, most other packages simply depend directly on - libjpeg-turbo.

zklaus commented 1 year ago

I think this is a Conda-smithy bug. I opened an issue at conda-forge/conda-smithy#1726. In brief, for linting, none of the Jinja variables are defined and they all get rendered as the empty string due to a Python 3 regression. For most, that's no problem because something else is there next to the {{ var }} bit, but for {{ libjpeg }} this is the only thing in the list and that's why it becomes None and breaks. With that fixed, the recipe lints fine, so I'd say you can ignore the linting error here.

mfansler commented 1 year ago

@zklaus thanks for looking into that!

dlaehnemann commented 1 year ago

Many thanks @mfansler and @zklaus! Those things were currently out of my league, and I learned a bit along the way. Cheers!