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

Update BH to 1.75.0 and minor regressions requiring downstream changes #76

Closed
4 of 6 tasks
eddelbuettel opened this issue Dec 13, 2020 · 37 comments
Closed
4 of 6 tasks

Comments

@eddelbuettel
Copy link
Owner

eddelbuettel commented Dec 13, 2020

Package BH has been updated to 1.75.0 (jumping from 1.72.0) and a release candidate is available for installation (see the brief tweet from yesterday, the command is a simple install.packages("BH", repos="https://ghrr.github.io/drat").

A reverse-dependency check (result summary is here) shows that a handful of packages need (often simple) changes:

I will file courtesy issue on the simple C++14 compilation change which I have verified in all three cases. I can also make PRs and possibly test that a change to C++14 does affect builds under current BH which I have not yet done. Both TDA and wellknown are still 'open' and I would very much appreciate it if the maintainers looked into it as well.

CRAN will close for the winter break on December 18 so it is unlikely we will get all this sorted out by then. CRAN reopens on January 4 and it would be fantastic if all packages could be updated by then allowing BH itself to migrate without breakage.

Please reach out (here or in email) if anything is unclear. I have access to one dedicated (if old/slow) machine where I can test this.

@Jean-Romain
Copy link

I tested rlas with BH 1.75.0 locally + tested on CRAN's winbuilder + with CI with BH 1.72. Everything was ok. I just submitted to CRAN. For lidR I'll do that in January. I'll keep you updated. Thanks.

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented Dec 13, 2020

Thanks a lot for the swift (and very positive) response! Yay C++14 😀

@Jean-Romain
Copy link

Hi again.

rlas is now on CRAN. I tested lidR with CXX_STD = CXX14 locally with BH 1.75.0 + with CI and CRAN's win-builder using BH 1.72.0. Everything looks good.

I planned to release a major version of lidR with many new features in January. Important changes are always a lot of work (review the documentation + a lot of unexpected troubles on CRAN despite CI). Could you give me a deadline for updating lidR. If I'm not able to get the major version ready in due date I'll update with a minor revision an delay the major version. Thanks.

@eddelbuettel
Copy link
Owner Author

Yes, thanks so much! I already 'ticked off' rlas at the top of the page.

"As soon as possible" would be my default answer but as you can tell it involves more than one package. "As fast as you can (to not hold anybody else back)" is probably more realistic.

I don't think I will update BH unless at least half of the packages have updated so there is some time. Now for you and @dcooley the change (imposed from this) is small (and mlpack is in the process of another upload where its small change is just part) but I do of course understand that any CRAN upload can, these days, be disruptive.

But do you think we can came early January when CRAN opens and be ready?

@Jean-Romain
Copy link

For a minor release yes it is not a problem. I could do that right now actually. But I'd prefer to release the major release and do not delay it for another 2 months if possible. I'll try to get the new release ready for early January. This was my plan anyway.

@eddelbuettel
Copy link
Owner Author

I failed to put a timely special thanks to @dcooley in here -- the updated version of googePolylines got to CRAN already yesterday. So two updated at CRAN, one more fix in its repo too (thanks @Jean-Romain !) -- keep the good news coming!

@eddelbuettel
Copy link
Owner Author

Oh, and of course one more thanks to @rcurtin as an updated mlpack is also already in the queue at CRAN (along with other changes).

@eddelbuettel
Copy link
Owner Author

And the updated mlpack is now on CRAN. Thanks, @rcurtin!

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented Jan 4, 2021

Hi @Jean-Romain I saw you committed to lidR so I presume a new upload is imminent now that CRAN is reopened? I can wait for that, and will keep an eye on CRANberries.

A new upload of BH will then follow.

@Jean-Romain
Copy link

Jean-Romain commented Jan 4, 2021

I submitted the package this morning at the first thing. However:

  • During the CRAN close days a new 'additionnal issue' appeared. This is a gcc-ASAN related stuff (meaning a lot of pain in the 7rd circle of the infernos). Hopefully it is a false positive that is related to rlas not directly to lidR. I hope this won't delay the submission.
  • This is a major release so I'm pretty sure I will run into unexpected troubles for systems I can't check such as... 32 bits solaris as always. And maybe MacOS.

I keep you up-to-date

@eddelbuettel
Copy link
Owner Author

Thanks for the update, and 100% agreed on the 7th circle of hell.

@eddelbuettel
Copy link
Owner Author

Thanks for the link. I see

==49310==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x60300013b760 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   32 bytes;
  size of the deallocated type: 16 bytes.
    #0 0x7f67452030cf in operator delete(void*, unsigned long) (/lib64/libasan.so.6+0xb30cf)

It seems to mismatch on different arches: 16 vs 32 bytes. You may be able to get to this one by switching unsigned int or unsigned short to explicit types such as uint32_t and alike.

@Jean-Romain
Copy link

Jean-Romain commented Jan 4, 2021

Thank you for your help. Appreciated. I roughly understand the problem from the report but didn't catch it is likely to be arches dependent. The biggest issues for me are actually:

  • it comes from a third party code base of more than 20,000 lines of low level C++ and I'm not the author this code.
  • the only way I'm able to reproduce an ASAN issue is using the rhub platform (I read everything you wrote about ASAN, USBAN and docker but..., well... 7rd circle of hell...) which render fixing tests long and unpleasant.
  • currently there is no issue in rlas so I need to reproduce independently of lidR (via rhub)

Anyway this is not the first time I fix such a low level issue like that in this library and I will eventually fix it. But it is never very fun.

@eddelbuettel
Copy link
Owner Author

Yes. Reproducing these is hell. I think I was the first in creating a suitable Docker container (and the rhub one is a descendant / uses it) but do not regularly update it (as it is beyond frustrating that BDR does not give us precise reproducible instructions ...) so the one I fall back when I need is part of Winston's meta-container with multiple builds which is generally excellent.

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented Jan 7, 2021

And now wellknown 0.7.2 (thanks, @sckott) but lidR remains in a holding pattern at incoming.

What's your expectation, if any, @Jean-Romain ? "Any day now" ?

@Jean-Romain
Copy link

I don't know. I submitted, Uwe Ligges asked me if gcc-ASAN issue was fixed. I answered that it was a false positive from rlas. Usually I get answer few hours later but it's been 4 days now. I don't know if they are expecting me to do something.

@eddelbuettel
Copy link
Owner Author

Yes, I have been there too. Sometimes all we can do is wait, as frustrating as it is. They are busy too and have churned out a 100+ releases since Monday, but the lack of information and insight is draining and tiring.

Very soon I will have the same coming with BH, RcppArmadillo and then Rcpp and there will undoubtedly be false positives and these rounds ... just waiting. I used to bug them more with emails inquiring, but that does not make it any faster either. The best solution I have is to shrug,and to also upload to a drat repo and just wait.

@Jean-Romain
Copy link

I'm not in a hurry. But you are waiting after me and sadly I can't do anything for you...

The problem with the CRAN is not the time it takes. We all understand that members are volunteers and there are a lot of packages to deal with and actually it is usually fast. But the absence of communication is frustrating when it does not run smooth and you don't know if you are supposed to do something or not. Anyway...

By the way I fixed the issue with gcc-asan for rlas. This issue was triggered after switching to c++14 but was not detected (or was not existing) with c++11and unit test in rlas were not extensive enough to detect it. I compiled the original library outside of R (I'm not able to make anything with ASAN/USBAN and R) and reproduced the issue successfully.

@eddelbuettel
Copy link
Owner Author

Well done! I often do the same (self-contained C++ snippets of code to minimize dependencies). Also make SAN orchestration so much easier. I am in the middle of something now but I a) believe I have prior write ups showing to debug something like this and b) would be up documenting this with you (as we know the error is there, now it is "merely" about instrumenting the tool).

@nx10
Copy link

nx10 commented Jan 10, 2021

Hi, Just checking in on the state of this. Will BH be updated when lidR gets accepted by CRAN? The TDA maintainers seem unresponsive.

I don't want to create a hurry, I just have to decide whether to make a submission with Beast vendored in, or to wait on BH.

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented Jan 10, 2021

Yes, that is exactly the state of matters:

  • BH will be updated
  • Everybody (apart from team TDA) did their part, so big thanks for that
  • lidR is stuck at CRAN though, sadly
  • RcppArmadillo is equally affected by lidR (and needs an update itself due to Matrix changes
  • but after five days I was tired of waiting and upload RcppArmadillo anyway, and am currently waiting on "recheck"
  • BH is next and I will upload it either today or tomorrow

Sadly, I would expect it to take maybe up to a week again. But that is the speed at which CRAN moves right now. Please hang in there just a tiny bit longer. We are almost there.

@nx10
Copy link

nx10 commented Jan 10, 2021

Thank you for your work and response. I will have to wait a bit, but what's one week or so more after all this time.

@eddelbuettel
Copy link
Owner Author

Now submitted. You can follow the incoming/ directory at CRAN or, say, the function in package foghorn (or this helper from littler wrapping it) to follow along:

edd@rob:~/git/bh(master)$ cranIncoming.r BH
 package  version cran_folder                time     size
      BH 1.75.0.0     pretest 2021-01-10 17:19:00 11881161
edd@rob:~/git/bh(master)$ 

Alessandro-Barbieri referenced this issue in gentoo/guru Jan 11, 2021
Package-Manager: Portage-3.0.12, Repoman-3.0.2
Signed-off-by: Theo Anderson <telans@posteo.de>
@Jean-Romain
Copy link

So, lidR was rejected based on the false positive asan error from rlas. It seems there is no way to argue. Thus, I submitted the fixed version of rlas (including more unit tests to trigger an error if still present). Sorry! I hope this won't imply a rejection for your packages as well.

@eddelbuettel
Copy link
Owner Author

BH was submitted and is in waiting. lidR is one of the issues (as we knew since December) as is TDA (which hasn't moved).

So it is entirely possibly that BH also gets rejected. We shall see.

@eddelbuettel
Copy link
Owner Author

eddelbuettel commented Jan 11, 2021

Thanks, on its way to CRAN.

My email was apparently a good enough explanation.

Edit: And here is the CRANberries tweet:

image

With rlas here I guess we get to see lidR soon too?

@eddelbuettel
Copy link
Owner Author

With that big big BIG Thank You! to everybody who updated their package to help with the transition to BH 1.75.0

Thanks also to @nx10 for the nudges for Boost Beast and the patience in waiting for this release to be made available.

@Jean-Romain
Copy link

Yes rlas passed quickly without human intervention. However I now need to wait until it is compiled on all platform I guess otherwise I will get the same false positive issue.

@eddelbuettel
Copy link
Owner Author

Actually, the incoming tests get the new versions immediately (and Uwe said so once on the r-package-devel list). Plus AFAIK only that Windows + Debian check matters for the initial check. That's how RcppArmadillo sailed through on Satuday. They still reserve the right to throw you off a few days later if, say, Solaris acts up or other unpleasantness. But don't have to wait for the results page to be complete.

@nx10
Copy link

nx10 commented Jan 12, 2021

No problem, and thank you again @eddelbuettel for enduring me checking in so often. I just submitted httpgd. Now hoping it will be accepted.

@eddelbuettel
Copy link
Owner Author

New packages take a little longer, but fingers crossed!

@Jean-Romain
Copy link

lidR has been rejected based on... I don't know... both Window and Debian pre-test were full OK. Maybe the new M1mac coming out of nowhere who knows. I will wait few more days until this error is erased by a compilation error generated by BH

@eddelbuettel
Copy link
Owner Author

Uggh. By "this eror" mean wait til the M1mac box has 1.75.0-0? You could depend on it in DESCRIPTION (which I do less often than I should as we can usually rely on CRAN to be current). It could also lead to hard error "cannot compile". Not 100% sure but might be worth a try. Or, as you say, chill and just wait...

@Jean-Romain
Copy link

lidR is finally on its way on CRAN.

@eddelbuettel
Copy link
Owner Author

Yes, thanks, just saw this too:

image

And I think I can close this now as BH has been on CRAN for a few days. Thanks again to everybody for the help!

@nx10
Copy link

nx10 commented Jan 19, 2021

httpgd is out now, too. Thanks for making this possible!

@eddelbuettel
Copy link
Owner Author

Congrats on getting httpgd onto CRAN. I will check this out as it looks nifty and may help with graphics from a plain terminal.

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

No branches or pull requests

3 participants