Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfixes mac.binary subtype handling accross insert/archive/prune functions #89
Conversation
getPathForPackage() reimplemented the function of utils::contrib.url and was thus dropped.
FelixErnst
changed the title
|
Thanks for the PR. It looks fine in principle though wholesale removal of a formerly used function always makes me a little nervous. @mbjones @jankatins Your names are still in the comment of the function this PR would remove / streamline over. The reasoning is good; but just in case you still use We may merge this and test it then but aren't in a terrible hurry. |
| @@ -1,11 +1,11 @@ | |||
| Package: drat | |||
| Type: Package | |||
| Title: 'Drat' R Archive Template | |||
| Version: 0.1.5 | |||
| Date: 2019-03-28 | |||
| Version: 0.1.6 | |||
eddelbuettel
May 29, 2020
Owner
It's minor but I tend to set those as these "final" numbers only when I prepare the release, so I would have called this 0.1.5.1 instead.
It's minor but I tend to set those as these "final" numbers only when I prepare the release, so I would have called this 0.1.5.1 instead.
FelixErnst
May 29, 2020
Author
Contributor
Changed this to 0.1.5.1. I am quite keen to get it to CRAN, since I tripped over this this morning. It would save me some time by not having to prepare a preliminary solution
Changed this to 0.1.5.1. I am quite keen to get it to CRAN, since I tripped over this this morning. It would save me some time by not having to prepare a preliminary solution
eddelbuettel
May 29, 2020
Owner
I can probably help with that especially if you can help with testing (i.e. I have no macOS binaries hanging around)
I can probably help with that especially if you can help with testing (i.e. I have no macOS binaries hanging around)
FelixErnst
May 29, 2020
Author
Contributor
Sure thing. I did all the testing on macOS Catalina and thats how I noticed the problem. The code in the PR works for R 3.6 and 4.0. I inserted packages, archived and pruned.
Do you have other test in mind?
Sure thing. I did all the testing on macOS Catalina and thats how I noticed the problem. The code in the PR works for R 3.6 and 4.0. I inserted packages, archived and pruned.
Do you have other test in mind?
eddelbuettel
May 29, 2020
Owner
The macOS case is, I think, by far the most complicated.
The macOS case is, I think, by far the most complicated.
mbjones
May 29, 2020
Contributor
@eddelbuettel I took a quick look at the proposed changes and agree they seem to improve the state of things. The original function was intended to address problems we encountered with MacOS, and it seems we missed some key functionality. So I'm all in support of fixing these issues while retaining the ability to install and upgrade mac packages from the CRAN-sanctioned locations. Thanks for reaching out.
@eddelbuettel I took a quick look at the proposed changes and agree they seem to improve the state of things. The original function was intended to address problems we encountered with MacOS, and it seems we missed some key functionality. So I'm all in support of fixing these issues while retaining the ability to install and upgrade mac packages from the CRAN-sanctioned locations. Thanks for reaching out.
| split_pkgtype <- strsplit(pkgtype,"\\.")[[1L]] | ||
| write_pkgtype <- | ||
| paste(split_pkgtype[seq.int(1L,min(2L,length(split_pkgtype)))], | ||
| collapse = ".") |
eddelbuettel
May 29, 2020
Owner
That is excessive indentation which, to me, prohibits readability. 100 columns are fine, most of us have wide-screen monitors.
That is excessive indentation which, to me, prohibits readability. 100 columns are fine, most of us have wide-screen monitors.
FelixErnst
May 29, 2020
Author
Contributor
Thanks have to go to Bioconductor's BiocCheck. I didn't even notice it ...
Thanks have to go to Bioconductor's BiocCheck. I didn't even notice it ...
the wrapper contrib.url2 is version aware, so that archivePackages and pruneRepo now respect the version information, if working on binary packages. defaults to getRversion()
| stop("'version' must be in the format 'X.Y' or 'X.Y.Z'.") | ||
| } | ||
| version <- paste(split_version[seq.int(1L,min(2L,length(split_version)))], | ||
| collapse = ".") |
nfultz
May 29, 2020
Contributor
If you convert version to a base::package_version() instead of as.character, you can replace all the string munging with
paste0(version$major, '.', version$minor)
If you convert version to a base::package_version() instead of as.character, you can replace all the string munging with
paste0(version$major, '.', version$minor)
FelixErnst
May 29, 2020
•
Author
Contributor
I thought about this, but there is no base R function I could find, which reports version$minor as 6 (for R 3.6). Therefore the default output is getRversion(), which returns 3.6.3 a special class, which is not character and thus has to be converted.
edit: ... and it has to be converted to 3.6. Otherwise we cannot use it.
I thought about this, but there is no base R function I could find, which reports version$minor as 6 (for R 3.6). Therefore the default output is getRversion(), which returns 3.6.3 a special class, which is not character and thus has to be converted.
edit: ... and it has to be converted to 3.6. Otherwise we cannot use it.
eddelbuettel
May 29, 2020
Owner
I missed that. I use that a lot too (somewhere even from configure. That S3 class package_version and its constructors, and added operators, is gold. Hidden jewel.
I missed that. I use that a lot too (somewhere even from configure. That S3 class package_version and its constructors, and added operators, is gold. Hidden jewel.
FelixErnst
May 29, 2020
Author
Contributor
I am happy to change that, if there is a function, which reports 3.6 or 4.0 and not 3.6.3 or 4.0.0
I am happy to change that, if there is a function, which reports 3.6 or 4.0 and not 3.6.3 or 4.0.0
FelixErnst
May 29, 2020
Author
Contributor
Ok, now I got it. Give me a moment
Ok, now I got it. Give me a moment
nfultz
May 29, 2020
Contributor
I was thinking something like this:
> sessionInfo()
R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 18.04.4 LTS
Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/atlas/libblas.so.3.10.3
LAPACK: /usr/lib/x86_64-linux-gnu/atlas/liblapack.so.3.10.3
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
attached base packages:
[1] stats graphics grDevices utils datasets methods base
other attached packages:
[1] colorout_1.2-2
loaded via a namespace (and not attached):
[1] compiler_3.6.3 tools_3.6.3
> version <- getRversion()
> version$minor
[1] 6
> paste0(version$major, '.', version$minor)
[1] "3.6"
I was thinking something like this:
> sessionInfo()
R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 18.04.4 LTS
Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/atlas/libblas.so.3.10.3
LAPACK: /usr/lib/x86_64-linux-gnu/atlas/liblapack.so.3.10.3
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
attached base packages:
[1] stats graphics grDevices utils datasets methods base
other attached packages:
[1] colorout_1.2-2
loaded via a namespace (and not attached):
[1] compiler_3.6.3 tools_3.6.3
> version <- getRversion()
> version$minor
[1] 6
> paste0(version$major, '.', version$minor)
[1] "3.6"
FelixErnst
May 29, 2020
•
Author
Contributor
done. I didn't realize, that there are these accessors, even though they are referenced in the examples.
Thanks for the checking that.
done. I didn't realize, that there are these accessors, even though they are referenced in the examples.
Thanks for the checking that.
eddelbuettel
May 29, 2020
Owner
Yes, high redundancy. getRversion() and alike give us the components. The S3 class gives us comparison.
Yes, high redundancy. getRversion() and alike give us the components. The S3 class gives us comparison.
…rapper Thanks to Neal Fultz
|
Thanks everybody for the feedback. This is looking promising enough, as do casual tests on the checked out PR branch. As @FelixErnst did some serious enough polishing and expressed a need for seeing this fixed -- coupled with the good standing at CRAN I think I will merge, roll it up as 0.1.6 and ship it. |
eddelbuettel
merged commit 60ace21
into
eddelbuettel:master
This comment has been minimized.
This comment has been minimized.
|
@eddelbuettel we have the same in https://github.com/Rdatatable/data.table/blob/d3bd408ab3c2f46365b9e0d94a508d7874034d01/.ci/ci.R#L5-L9 |
This comment has been minimized.
This comment has been minimized.
|
I have no magic pixie dust either. Best is to prepare a patch, and convince an R Core member to apply it. |
|
And now on CRAN -- thanks so much everybody for help and input, and especially to @FelixErnst for a patiently revised PR. Let's hope it does the trick for you mac heads; if need be we can always refine once more. |
|
Thanks for the quick and helpful comments @eddelbuettel @nfultz and for the push to CRAN. I will continue to keep an eye on this, since I have to supply packages to mac users. |
Fixes #88