Warning will not throw again now that it's captured in base_messages#6146
Warning will not throw again now that it's captured in base_messages#6146MichaelChirico merged 4 commits intomasterfrom
Conversation
|
Am Mon, May 20, 2024 at 11:35:12AM -0700 schrieb Michael Chirico:
@tillea are you able to test that this PR resolves the issue?
Hmmmm, testing from a packaging point of view is quite some effort. We
are packaging from released tarball which contains the file bz2
compressed. Applying the patch involves uncompressing, patching and
compressing again which I need to do manually instead of just dropping
the patch at some defined location. Given that I'm on vac for two weeks
I have no capacity for this. If you might consider a new minor release
I could simply upgrade which could be done in a couple of minutes.
Thank you for the promising patch in any case, Andreas.
|
|
Thanks Andreas, could you elaborate on the urgency of a fix here? My instinct is to just include this PR with the next data.table release rather than triggering a patch release. That would happen sometime in July. |
|
Am Mon, May 20, 2024 at 02:18:43PM -0700 schrieb Michael Chirico:
Thanks Andreas, could you elaborate on the urgency of a fix here?
Its pretty urgent since data.table has a lot of dependencies and these
are all in trouble currently.
My instinct is to just include this PR with the next data.table release rather than triggering a patch release. That would happen sometime in July.
It would be of great help if you could release something in May.
Thanks a lot for your cooperation, Andreas.
|
tdhock
left a comment
tdhock
left a comment
There was a problem hiding this comment.
I'm not sure I understand -- is there some way we can alter our CI to catch this kind of issue, or is it too obscure?
Do I understand that this only affects 32-bit architectures? Is there an issue on another tracker we might be missing? It would seem that there's been no other requests to have this issue addressed in 2 months since 1.15.4 was released. Are we sure this is affecting many users? If I'm not mistaken, and please correct me if I'm wrong, but that test is the same on 1.15.0, so that would make it since Jan 30 that we haven't seen any other issues filed. Or perhaps there was only an attempt to ship 1.15.0+ version of data.table on your end starting very recently & it's not picked up automatically from CRAN updates? |
|
I can apply to two-line patch to the (Debian-internal) git repo of the package. We apply upstream (interim) all the time time so I am not sure I understand why a new release would be required to fix a Debian-only issue (on a non-release architecture). PS 1: The main problem is that I of course cannot test if the patch by @MichaelChirico actually fixes the Debian issue as I do not have (easy) access to a i386 system. But as the logic seems sound I may make sense to commit and push and let the Debian autobuilder have at it, including on i386. PS 2: Ah. I missed that the test file is bz2 compressed. PS 3: Monkey-patched in the tests.Rraw.bz2 file (and learned about PS 4 Builds worked fine -- screenshot for posterioty from the usual URL. We will have to wait and see what the autopkgtests say.
|
|
Uggh. There is apparently another autotest regression on armhf, see #1071737. |
|
@eddelbuettel sorry to bother you, but can you also include #6137 in your patch? This should do the trick! |
|
Yes I realized that that was the likely course of action. But will await test result on first patch. |
|
Looks like it failed again on i386: https://ci.debian.net/packages/r/r-cran-data.table/testing/i386/47062403/ This is not my package, and I (by my choice) do not run autopkgtest on the 100 or so CRAN packages I look after in Debian as I trust CRAN. So if it were my package I'd turn the autopkgtests off. |
|
@ben-schwen et al: Advice on how to uncork the current autopkgtest error on i386? I have a hard time reading the (somewhat non-standard) test runners you use and I am also unfamiliar with the autopkgtest framework (as I do not bother with it for the CRAN packages I maintain; I trust CRAN instead). Advice would be appreciated, the package still has a target data of June 11 on its back for removal from Debian unstable. |
|
@eddelbuettel AFAIU here is the timeline:
I resetted the test to its initial behavior and also tested thins a conda environment as suggested by @jameslamb in #5867 (comment) However, I can't reproduce the TZ problem reported there (the newly created conda environment gave me |
|
( Debian uses R 4.4.0, not 4.2.3 ) What changes do you suggest I monkey-patch into the bz2'ed test file? (I appreciate all the historical context, but the only thing that will move the needle is to not fail on i386 (as armhf seems to behave now?).) |
|
I assumed we are patching from
|
|
Missed the branch. If I may, you are looking at the wrong page. While Debian has 1.14.10 in testing the fight we are fighting here is on the belatedly-added (after I filed a bugreport) 1.15.4 (and .0 and .2 where skipped) which is now failing the required steps (as the 'autopkgtests' are unabled) in 'unstable' to migrate to 'testing' where it would replace 1.14.10. Pages I have open include
There is also a handy one-line summary at the QA page I usually go to for my packages https://qa.debian.org/developer.php?login=edd@debian.org -- and data.table is now there in the middle under r-cran-data.table as I 'touched' it as an uploader as the actual maintainer group seems too busy or uninterested. |
|
Thanks @ben-schwen -- I 'manually' adjusted the already patches file to that new branch and uploaded a new version. We should know in a day or two if this gets us over the goal line. |
@eddelbuettel there seems to be a typo ( |
|
@ben-schwen Yes, just went checking myself (before checking GitHub notifications). So that is likely me having fat-fingered it? If so can fix. (It is now named 1.15.4-2.1 as I opted for a so-called non-maintainer upload so more clearly signal that I am not really involved with the package.) PS I'll just pull your whole file instead of making the change by hand. PPS Yup. My brown-bag fail. Fix will be up in a minute. edd@rob:~/deb/data.table/inst/tests(master)$ diff -u tests.Rraw.prev tests.Rraw
--- tests.Rraw.prev 2024-05-29 15:40:26.000000000 -0500
+++ tests.Rraw 2024-05-30 07:04:24.957697194 -0500
@@ -17849,7 +17849,7 @@
test(2226.1, base::prod(2147483647L,2L), 4294967294) # just to illustrate that base returns double
DT = data.table(x=c(lim.integer64(), 2, 1, NA, NA, -2, 4), g=INT(1,2,1,2,1,2,3,3))
test(2226.2, DT[, prod(x), g], data.table(g=1:3, V1=as.integer64(c(NA,NA,-8L))))
- if (test_longdouble) ttest(2226.3, DT[, prod(x,na.rm=TRUE), g], data.table(g=1:3, V1=as.integer64(c(NA,"9223372036854775807",-8L))))
+ if (test_longdouble) test(2226.3, DT[, prod(x,na.rm=TRUE), g], data.table(g=1:3, V1=as.integer64(c(NA,"9223372036854775807",-8L))))
}
# set ops when DT has column names x and y, #5255
edd@rob:~/deb/data.table/inst/tests(master)$
``` |
|
@ben-schwen That worked -- the autopkgtests passed and the package is now back in 'testing'. So big thank you for all the help. Will you merge the change for any next release? |
|
The armhf fix is already in master #6137 (we found that one 2 weeks ago) and once this PR is merged, both of the manual patches will be in the next release. @eddelbuettel I saw that we are failing on loong64 due to libcurl4, is that something we can ignore? |
|
Yes, apparently looong64 is not what is called a 'release architecture' so the fail there does not matter. The transition actually happened today (but not without at the last moment triggering mass email to the maintainer of all packages dependending on r-cran-data.table because of 'pending autoremoval on June 11' -- I got ten or so). So I think we really are good and done here. Thanks a bloody much for the help (and my vote for keeping the file in the source tarball uncompressed as in the repo). |
|
Am Sat, Jun 01, 2024 at 09:21:23AM -0700 schrieb Dirk Eddelbuettel:
Thanks a bloody much for the help (and my vote for keeping the file in the source tarball uncompressed as in the repo).
Thanks to you both (and I also like the uncompressed file in tarball). Andreas.
|
|
Thanks a ton @eddelbuettel for the assist here, your expertise is much appreciated.
Yes, this is a CRAN thing to keep our binary a bit smaller. We still get the "5MiB" Is there any way to ship to Debian a different binary than CRAN (Or is that a bad idea anyway)? Or maybe we should revisit just saying "don't care" about the "too large" CRAN scold. |
|
Merging this to |
A package maintainer can easily accomodate such differences in the installed package but we generally take sources as given. And it is at that source level that it matters when you all decide to stick a binary into a compressed tar. That is generally considered bad taste. So I would just eat the NOTE (as I do on large packages). |
Patch from upstream: Rdatatable/data.table#6146
5 participants
Closes #6133
Since the warning from R is only thrown once per session, there will be no
warning=to match, since the warning is already generated whenbase_messages$maybe_invalid_old_posixctis thrown.@tillea are you able to test that this PR resolves the issue?