r-lidar / rlas

R package to read and write las and laz files used to store LiDAR data
https://cran.r-project.org/package=rlas
GNU General Public License v3.0
34 stars 14 forks source link

Include array header to appease g++-12 #57

Closed eddelbuettel closed 1 year ago

eddelbuettel commented 1 year ago

While testing an upgrade of package BH against all its reverse-dependencies (see this issue for details) I noticed that rlas failed to build.

The cause is unrelated to the BH upgrade and due to the fact that I ran the tests with g++-12 (on Debian testing). The newer compiler wants an explicit #include for the std::array data structure. Once that one line is added the package installs and tests fine under g++-12.

The change is very simple. It would be lovely if you could update the package at CRAN even though this is more of a g++ issue than a BH issue.

Jean-Romain commented 1 year ago

Thanks. I need to fix numerous new errors that spontaneously appeared on CRAN before to update rlas. I will do that on Monday hopefully.

eddelbuettel commented 1 year ago

Yes, the sprintf -> snprintf is annoying. I have done it in two packages.

Thanks for the swift merge though!

Jean-Romain commented 1 year ago

Hi @eddelbuettel ,

Since we started the discussion about sprintf I'm wondering if you could kindly give me some tips before I ask on Stackoverflow? First I've seen nothing about sprintf in Writing R extensions. Second I have 784 occurences from the third party library included. I tried a naive automatic replacement with grep and sed but of course the modification are non trivial and most are failing. For example this is failing for obvious reasons.

  inline I32 get_command(CHAR* string) const { return snprintf(string, sizeof(string), "-%s %g %g %g ", name(), ll_x, ll_y, tile_size); };

Did you do it by hand or did you write an automatic replacement script? If I have to do it by hand I'm probably better to comment most of the code automatically. I'm pretty sure most is useless.

Thanks

eddelbuettel commented 1 year ago

I think this may be documented in the r-devel branch of the docs, have you check there rather than in the one from the previous 4.2.2 release? The handy link from RStudio / posit has the devel one prettily formatted: https://rstudio.github.io/r-manuals/

As for the changes, I 'did it by hand' which was a bit crazy but still doable for AsioHeaders, easy for digest and something I am now afraid of for Boost where I have not done it. It is too important a library to make those changes so I push back if they ask.

It is once again a 'we know where they are coming from, and it is the right move in the long run' but still a pain to do :roll_eyes:

Jean-Romain commented 1 year ago

Thank you for your reply. So, I won't be able to release rlas asap. The amount of work to be done is significant.

I greped some stuff and I'd say I can probability do 285/785 occurrence by commenting unused code and 305/785 automatically. It remains 195 occurrences spread in 60.000 lines of code that need more careful investigation.

eddelbuettel commented 1 year ago

I hear you. In that case if it were me I might consider a first, narrow, micro release adding the one-line from this PR (and possibly other pending issues) to get CRAN update and then work on the (more painful, more work, but as of right now not urgent snprintf transition. Just a thought.

Jean-Romain commented 1 year ago

I do agree with that plan but if my submission is rejected by the CRAN I think I won't be in position to negotiate. Anyway, I'm submitting and give you feed back later.

eddelbuettel commented 1 year ago

That's a fair point! I have only submitted sprintf fixes recently, and not packages that may be held up.

Jean-Romain commented 1 year ago

Rejected. I sent an email to negotiate. I mentionned your name to (maybe) get more weight :wink:.

eddelbuettel commented 1 year ago

Thanks for shepherding 1.6.2 onto CRAN -- much appreciated!