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

write_PACKAGES and archive/pruneRepo work on different paths for mac.binary #88

Closed
FelixErnst opened this issue May 28, 2020 · 10 comments · Fixed by #89
Closed

write_PACKAGES and archive/pruneRepo work on different paths for mac.binary #88

FelixErnst opened this issue May 28, 2020 · 10 comments · Fixed by #89

Comments

@FelixErnst
Copy link
Contributor

Hi,

I encountered the following situation:

  • drat::insertPackage("package_0.2.11.tgz","~/repo") works
  • drat::insertPackage("package_0.2.12.tgz","~/repo", action="archive") reports the following error
Error in order(NULL, integer(0), na.last = TRUE, decreasing = TRUE, method = "auto") : 
  argument 1 is not a vector

This originates in pruneRepo via archiveRepo. The reason for this is that, the package binary gets written to bin/macosx/el-capitan/contrib/3.6/, but the pruning is trying to work on bin/macosx/contrib/3.6.

Before preparing a PR, I think this needs to be discussed, for me to understand the differences, since I am new to the inner workings of the repo structure R expects.

My initial starting question was: Why was the wheel reinvented using the the function getPathForPackage in insertPackage, when contrib.url is already available and used in pruneRepo?

reldir <- getPathForPackage(file)

and the next line could also be written as contrib.url(repodir, type = pkgtype) and probably solve the issue. Answer: The el-capitan subdir is there for a reason.

So the real starting question is: For what reason is the el-capitan subdir required and when does R expects it to be present?

Thanks for any advice and hints in advance.

SessionInfo
R version 3.6.3 (2020-02-29)
Platform: x86_64-apple-darwin15.6.0 (64-bit)
Running under: macOS Catalina 10.15.4

Matrix products: default
BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib
LAPACK: /Library/Frameworks/R.framework/Versions/3.6/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats graphics grDevices utils datasets methods base

other attached packages:
[1] drat_0.1.5

loaded via a namespace (and not attached):
[1] compiler_3.6.3 tools_3.6.3

@eddelbuettel
Copy link
Owner

Nice analysis! And totally agree that we should discuss this, and then hopefully clean it up.

Why was the wheel reinvented

Excellent question. Possibly via a contributed extension that was unaware the earlier function.

For what reason is the el-capitan subdir required and when does R expects it to be present?

drat tries to provide a structure that another user can access via install.packages() and update.packages(). And AFAIK on macOS the 'version' always gets injected in the path because the different release are not generally (or not always) compatible.

That is probably also a like issue with the archive error you noticed. The code may be unaware that it is working on macOS tree (just a guess, could be wrong).

Looking http://cloud.r-project.org/bin/macosx/ suggests all sorts of subdirectories:

  • el-capitan/contrib/3.{456}
  • mavericks/contrib/3.{23}
  • contrib/4.0
  • contrib/r-release

Maybe we can draw a line and redefine for 4.0?

@FelixErnst
Copy link
Contributor Author

I had a look at the structure on CRAN as well. I think this might reflect the tool chains used in the past for macOS.

Can you confirm this output:

> contrib.url("test",type = "source")
[1] "test/src/contrib"
> contrib.url("test",type = "mac.binary")
[1] "test/bin/macosx/contrib/4.0"
> contrib.url("test",type = "mac.binary.el-capitan")
[1] "test/bin/macosx/el-capitan/contrib/4.0"
> contrib.url("test",type = "mac.binary.mavericks")
[1] "test/bin/macosx/mavericks/contrib/4.0"
> contrib.url("test",type = "win.binary")
[1] "test/bin/windows/contrib/4.0"
> contrib.url("test",type = "binary")
[1] "test/bin/windows/contrib/4.0"

The last output is platform specific, since it resolves the type with .Platform$pkgType in utils:::resolvePkgType.

However, for mac.binery.x contrib.url behaves in a special way:

> contrib.url("test",type = "mac.binary.x")
[1] "test/bin/macosx/x/contrib/4.0"

So the question is how can the os flavour be extracted from a mac binary file to get pkgtype? drat:::getPackageInfo tries to do this, but some of the fields in the build string are just empty.

Would a simple switch on the major R release work? This is suggested by the folder structure on CRAN isn't it?

@FelixErnst
Copy link
Contributor Author

FelixErnst commented May 29, 2020

So my guess would be that a simple switch should work. Switching on Catalina from 3.6 to 4.0 resulted in the el-capitan subdomain to be dropped. Switching back to 3.6 reinstated it. So I guess the subdir is not to match the users macOS version, but is used as identifier for a build chain matching the R version.

@eddelbuettel: Do you agree? If so, expect a PR in the evening (CET).

edit: for testing purposes PR was submitted.

@eddelbuettel
Copy link
Owner

eddelbuettel commented May 29, 2020

It's complicated. Should we detect the OS from the users system? What happens if I (on Linux) try to drat::insertPackage() a binary I made on RHub? Should the last word not be the file? But can we detect all from it (on all platforms)?

The "structure" above looks fine from eye-balling. But I don't have a mac so not sure how much my eye-balling matters...

@FelixErnst
Copy link
Contributor Author

I also noticed, that contrib.url uses the R version info to construct the path, which as you said must not match the package version.

In any event the whole thing must match accross all functions (insert/archive/prune). Otherwise, there is no point. I prepare any additional commit for what I have in mind.

@eddelbuettel
Copy link
Owner

eddelbuettel commented May 29, 2020

In any event the whole thing must match accross all functions (insert/archive/prune).

Completely agree. That would be the ideal behavior. I think we also have that for source 😉

@FelixErnst
Copy link
Contributor Author

Well, there is not version identifier for source, so I guess, that isn't affected, is it?

@eddelbuettel
Copy link
Owner

eddelbuettel commented May 29, 2020

That was a tongue-in-cheek joke. There is only one structure for sources on CRAN repos.

@FelixErnst
Copy link
Contributor Author

😄 that went over my head.

I added a wrapper for contrib.url. The contrib_url is now always a result of this wrapper, so we can tweak the output at a single point, if necessary

@eddelbuettel
Copy link
Owner

Taken care of on #89

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 a pull request may close this issue.

2 participants