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

Reuse the provided dllname #162

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Reuse the provided dllname #162

merged 2 commits into from
Jun 3, 2021

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Jun 1, 2021

We have a package that due to shared library name collisions we had to rename the generated .so to something else. In that case we rename it from torch.so to torchpkg.so.

A recent change in #133 uses a temporary file that has the same name as the package

https://github.com/r-lib/pkgload/pull/133/files#diff-33e8edea32dfc4630f62c402513e58979a315710778bc94dde36fb56c942a80eR92-R95

But this causes problems when the DLL being loaded doesn't have the same name as the R package.

This PR proposes to reuse the DLL name that was provided to library.dinam2 to fix the problem.

@jimhester jimhester requested a review from lionel- June 1, 2021 19:43
@jimhester
Copy link
Member

Seems fine to me, @lionel-, it would be good if you could review this, and @dfalbel
can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@dfalbel
Copy link
Contributor Author

dfalbel commented Jun 2, 2021

Thanks @jimhester, added the NEWS bullet.

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

Makes sense!

@jimhester jimhester merged commit 04cd670 into r-lib:master Jun 3, 2021
@jimhester
Copy link
Member

Thanks @dfalbel for the PR and @lionel- for reviewing!

@krlmlr
Copy link
Member

krlmlr commented Jun 5, 2021

For reasons I don't fully understand, only this version works correctly with pillar on the M1. No action needed, except for a speedy CRAN release ;-)

With dev pkgload:

pkgload::load_all("~/git/R/pillar")
#> ℹ Loading pillar
tibble::tibble(a = 1)
#> # A tibble: 1 x 1
#>       a
#>   <dbl>
#> 1     1
pkgload::load_all("~/git/R/pillar")
#> ℹ Loading pillar
tibble::tibble(a = 1)
#> # A data frame: 1 × 1
#>       a
#>   <dbl>
#> 1     1

Created on 2021-06-05 by the reprex package (v2.0.0)

With pkgload v1.2.1, either from CRAN or from a local clone:

pkgload::load_all("~/git/R/pillar")
#> ℹ Loading pillar
tibble::tibble(a = 1)
#> # A tibble: 1 x 1
#>       a
#>   <dbl>
#> 1     1
pkgload::load_all("~/git/R/pillar")
#> ℹ Loading pillar
tibble::tibble(a = 1)
#> # A data frame: 1 × 1
#>       a
#>   <dbl>
#> 1     1

Created on 2021-06-05 by the reprex package (v2.0.0)

@krlmlr
Copy link
Member

krlmlr commented Jun 5, 2021

I stand corrected: both version behave the same, I'll file an issue.

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.

4 participants