Please do not open the Rcpp namespace for all about rcpp_progress HOT 33 CLOSED

@kforner I encountered this other day, and believe it is unrelated to anything discussed in this ticket. As far as I can tell, since submitting your previous version of RcppProgress to CRAN (which necessarily must have built on Windows), you have added the file progress_bar.hpp, which has the only occurrence of #include <Rinterface.h>. From the cursory 5 minute Google search I did, I got the impression that this aspect of the R API is not available on Windows (I'll try to track down this source again).

Edit: In Writing R Extensions,

Rinterface.h: for add-on front-ends (Unix-alikes only)

from rcpp_progress.

You could probably do something like

#if !defined(WIN32) && !defined(__WIN32) && !defined(__WIN32__)
// your current progress_bar.hpp code
#endif

Presumably you are calling this functionality from R (and possibly other places in C++); so will likely have to take into account the OS in those places as well.

from rcpp_progress.

I was 'off the grid' while commuting to work / catching up on other things.

@nathan-russell is spot on re Rinternals.h. We designed Rcpp.h such that you should not need to add further includes.

Changing package sglOptim as suggested also seems like a good idea; that is where this issue hit us.

As for the

The last think I want is to break other packages.
What would your recommend me to do ?

I can work with you and run rev.dep tests. I think you should make the change, maybe if you prefer that "phased in" but we should narrow this down. The package has a decent-but-manageable list of reverse depends and I have scripts here we can use / adapt. I like the idea of RcppProgress and would like to help. So ... deal ?

from rcpp_progress.

All R packages should have a valid Maintainer entry in the DESCRIPTION file (Maintainer: Maarten Kruijver <[email protected]>, so just contact them and let them know that you will be removing the using namespace Rcpp; declaration from your header file(s). Fortunately, this will most likely be a very trivial change for all such dependent packages, and any files currently including progress.hpp should only need to add using namespace Rcpp; locally to get the same behavior. It is not uncommon to notify authors of dependent packages of breaking changes upstream, and all things considered,

• this is about as justifiable of a breaking change as it gets; and
• the modification required on their end is about as minimal as it gets

from rcpp_progress.

ok. fixed, tested on rhub and submitted to CRAN.

from rcpp_progress.

Narrowing this down further, I think it is specifically due to <Rinternals.h> being included before <Rcpp.h> in this package. For example,

#include <Rinternals.h>
#include <Rcpp.h>

// [[Rcpp::export]]
bool is_null(Rcpp::Nullable<Rcpp::NumericVector> x) {
    return x.isNull();
}

C:/R/R-3.3.1/library/Rcpp/include/Rcpp/Nullable.h:108:36: note: candidate is:
In file included from isNull.cpp:1:0:
C:/R/R-33~1.1/include/Rinternals.h:1192:18: note: bool Rcpp::Nullable::Rf_isNull() const [with T = Rcpp::Vector<14, Rcpp::PreserveStorage>]
#define isNull Rf_isNull
^
C:/R/R-3.3.1/library/Rcpp/include/Rcpp/Nullable.h:106:21: note: in expansion of macro 'isNull'
inline bool isNull() const {
^
C:/R/R-33~1.1/include/Rinternals.h:1192:18: note: candidate expects 0 arguments, 1 provided
#define isNull Rf_isNull
^
C:/R/R-3.3.1/library/Rcpp/include/Rcpp/Nullable.h:106:21: note: in expansion of macro 'isNull'
inline bool isNull() const {
^

On changing the #include order, it compiles:

#include <Rcpp.h>
#include <Rinternals.h>

// [[Rcpp::export]]
bool is_null(Rcpp::Nullable<Rcpp::NumericVector> x) {
    return x.isNull();
}

However, Rcpp already does include <Rinternals.h> (in the correct order), so that shouldn't be needed in most (if not all) situations:

#include <Rcpp.h>

// [[Rcpp::export]]
bool is_null(Rcpp::Nullable<Rcpp::NumericVector> x) {
    return Rf_isNull(x.get());
}

While this error does happen with or without using namespace Rcpp; I would strongly, strongly, strongly encourage you to find an alternative to putting

#include <Rcpp.h> 
using namespace Rcpp; 

in a header file.

from rcpp_progress.

Hi,

I agree that it is not good, but looking at the comment just above: "// I must keep this because now some dependent packages rely on Rcpp namespace to be available"
makes me think that I already tried to remove it and that it broke dependent stuff.
The problem is that I do not remember at all when and what packages are concerned.

The last think I want is to break other packages.
What would your recommend me to do ?

from rcpp_progress.

I don't know if it will create any other issues, but adding #define R_NO_REMAP directly above #include <Rinternals.h> in this file should fix this, as the following now compiles:

#define R_NO_REMAP
#include <Rinternals.h>
#include <Rcpp.h>

// [[Rcpp::export]]
bool is_null(Rcpp::Nullable<Rcpp::NumericVector> x) {
    return x.isNull();
}

from rcpp_progress.

@eddelbuettel: what do you think ?

from rcpp_progress.

Another possible option would for the sglOptim package to make the following adjustments:

• in sgl.h, use

  #define R_NO_REMAP
  #include <progress.hpp>

• and in rObject_def.h change the top of the file to be

  #ifndef ROBJECT_DEF_H_
  #define ROBJECT_DEF_H_
  
  #define mkChar Rf_mkChar

Making these changes, the package seems to compile correctly for me, but that is something you would have to discuss with the maintainer of sglOptim. CC @vincent-dk

from rcpp_progress.

deal ;). So what is the process ?

from rcpp_progress.

How about:

• You make changes as you see fit.
• I was thinking (while commuting in) that you could keep the current header as a renamed one, say RcppProgress_legacy.h or something so that clients would not need to do anything other than including that one.
• At the same time we tighten RcppProgress.h, make a minor (GH) release with a different version number (to get a different tarball), I test that.

And if all is well you ship to Vienna.

from rcpp_progress.

I made the changes.
I can already tell you that it breaks DNAprofiles 0.3.1:

dbcomparepairwise.cpp:6:1: error: ‘NumericMatrix’ does not name a type
 NumericMatrix Zdbcomparepairwise(IntegerVector db, int nloci,bool display_progress=true) {
 ^~~~~~~~~~~~~
dbcomparepairwise.cpp:62:1: error: ‘List’ does not name a type
 List Zdbcomparepairwisetrackhits(IntegerVector db, int nloci,int hit,bool display_progress=true) {
 ^~~~

from rcpp_progress.

I will turn this on once I get to work, and we will know a little more then.

Can I just grab the version from the repo?

from rcpp_progress.

from rcpp_progress.

And as I mentioned earlier, maybe a peace offering progress_legacy.hpp they could switch to which still has the old behaviour? But what @nathan-russell says is all true. They only need to add a line, and it does belong into their compilation units.

from rcpp_progress.

I got distracted and only got to this more recently. In short I get

               pkg res
1              CFC   0
2            cpgen   0
3           dfcomb   0
4            dfmta   0
5      DNAprofiles   1
6             GCPM   1
7           JAGUAR   0
8         largeVis   0
9             lsgl   1
10 matchingMarkets   0
11            msgl   1
12            NNLM   0
13   rFTRLProximal   0
14        sglOptim   1
15       textmineR   0
16       textreuse   0

 0  1 
11  5 
[1] "DNAprofiles" "GCPM"        "lsgl"        "msgl"        "sglOptim"   

DNAprofiles you already knew, GCPM looks similar. I will try both of them locally with the legacy header you kindly prepared.

The other three are all Martin Vincent's nexus and he has new versions of all three pending, so I do not think we need to worry about them.

Let me know if you want / need fuller logs.

from rcpp_progress.

DNAprofiles and GCPM both pass once you replace #include <progress.hpp> with #include <progress_legacy.hpp> in one location each.

So I'd say you can upload and kindly ask the two maintainers to update their packages, possibly with a slight hint that wide open namespaces are not a great idea (but both seem small so they are safe). Also send a note to Martin but I think he knows.

And we all are making 'library-level' code (ie your package) better. A win.

from rcpp_progress.

I have made the suggested adjustments to the github version of sglOptim, planing to submit to CRAN this year. Let me know if there is any further issues with any of the sgl packages.

from rcpp_progress.

@vincent-dk Thank you.

from rcpp_progress.

@eddelbuettel : my package seems to fail under windows using https://win-builder.r-project.org:

*** arch - i386
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R-3.3.2/include" -DNDEBUG    -I"d:/RCompile/CRANguest/R-release/lib/Rcpp/include" -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp -I../inst/include   -O2 -Wall  -mtune=core2 -c tests.cpp -o tests.o
In file included from ../inst/include/interruptable_progress_monitor.hpp:17:0,
                 from ../inst/include/progress.hpp:13,
                 from tests.cpp:1:
../inst/include/progress_bar.hpp:13:24: fatal error: Rinterface.h: No such file or directory
 #include <Rinterface.h>

Do you have an idea why ? Should I submit it nonetheless ?

from rcpp_progress.

No you should probably clean that up. Nobody changed anything in the -I flags in src/Makevars so that is puzzling.

Do you have a Windows machine to test? I don't.

from rcpp_progress.

It also just failed for me on R Hub with check_on_windows().

from rcpp_progress.

Your src/Makevars is outdated (we stopped requiring Rcpp::LdFlags() several years ago).

Maybe adding a src/Makevars.win would help. I don't know. For Rcpp all we set is

edd@max:~$ cat git/rcpp/src/Makevars.win 
PKG_CPPFLAGS =  -I../inst/include/

edd@max:~$ 

It seems like R is confused now if it no longer finds its headers. Maybe "" vs <> semantics changed. I really don't know. I just rebuilt Rcpp on win-builder yesterday. But it looks like we never include Rinterface.h.

Sorry, have no immediate help to suggest.

from rcpp_progress.

ok. I used Rinterface just for R_FlushConsole. I guess I can omit for windows, it does not break the functionality. What the proper way to detect the Windows platform in my header please ?

from rcpp_progress.

Damn, I had forgotten to confirm the submission to CRAN !

from rcpp_progress.

The needed updates for sglOptim was put on CRAN last year. Note: sglOptim has an unrelated issue on solaris-x86, but I don't think this should cause any problems in relation to this update.

from rcpp_progress.

@vincent-dk perfect, thanks a lot.

from rcpp_progress.

I formally object to these comments being deleted here. I did not make these comment for you to delete them. But let me just state that you are doing your utmost to de-motivate me.

Date: Wed, 04 Jan 2017 05:00:01 -0800                                                                                                                                                                               
From: Dirk Eddelbuettel <[email protected]>                                                                                                                                                                  
To: kforner/rcpp_progress <[email protected]>                                                                                                                                                        
Cc: Dirk Eddelbuettel <[email protected]>, Your activity <[email protected]>                                                                                                                            
Subject: Re: [kforner/rcpp_progress] Please do not open the Rcpp namespace for all (#3)                                                                                                                             
Reply-To: kforner/rcpp_progress <reply+000a4561dcf25f37f85b6ba5796068ec1850ba6516a3595592cf000000011484b3d192a169ce0ba35fc4@reply.github.com>                                                                       
                                                                                                                                                                                                                    
@kforner Can you provide an update as to why the package is not on CRAN?                                                                                                                                            
                                                                                                                                                                                                                    
I asked for an (arguably rather trivial) change a full three weeks ago. Even taking holidays and everything else into account, isn't that enough time to change a line or two?                                      
                                                                                                                                                                                                                    
--                                                                                                                                                                                                                  
You are receiving this because you are subscribed to this thread.                                                                                                                                                   
Reply to this email directly or view it on GitHub:                                                                                                                                                                  
https://github.com/kforner/rcpp_progress/issues/3#issuecomment-270364985   

and

Date: Wed, 04 Jan 2017 06:28:53 -0800                                                                                                                                                                               
From: Dirk Eddelbuettel <[email protected]>                                                                                                                                                                  
To: kforner/rcpp_progress <[email protected]>                                                                                                                                                        
Cc: Dirk Eddelbuettel <[email protected]>, Your activity <[email protected]>                                                                                                                            
Subject: Re: [kforner/rcpp_progress] Please do not open the Rcpp namespace for all (#3)                                                                                                                             
Reply-To: kforner/rcpp_progress <reply+000a45616f4e80b1cfaca374fa90353f61dd9a68b63a379692cf000000011484c8a592a169ce0ba35fc4@reply.github.com>                                                                       
                                                                                                                                                                                                                    
> but it was not accepted on CRAN                                                                                                                                                                                   
                                                                                                                                                                                                                    
That is not how I understand that CRAN works.                                                                                                                                                                       
                                                                                                                                                                                                                    
Moreover, Martin uploaded his packages on December 30 as he promised he would.                                                                                                                                      
                                                                                                                                                                                                                    
I still do not understand what you are waiting for and find this very, very disappointing.                                                                                                                          
                                                                                                                                                                                                                    
--                                                                                                                                                                                                                  
You are receiving this because you are subscribed to this thread.                                                                                                                                                   
Reply to this email directly or view it on GitHub:                                                                                                                                                                  
https://github.com/kforner/rcpp_progress/issues/3#issuecomment-270382906    

from rcpp_progress.

package resubmitted to CRAN.

from rcpp_progress.

Thank you for the update.

from rcpp_progress.

added example and resubmitted again to CRAN.

from rcpp_progress.

on CRAN now.

from rcpp_progress.