- 
                Notifications
    
You must be signed in to change notification settings  - Fork 161
 
fix(r/adbcbigquery,r/adbcflightsql,r/adbcsnowflake): fix warnings on R CMD check for Go-based drivers #3061
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
Conversation
| 
           (I just wanted to make sure this solution works)  | 
    
| 
           @paleolimbot As far as I can see the difference in the logs between adbcsnowflake (nothing) and adbcflightsql (changed), does this change seem to work well? https://github.com/apache/arrow-adbc/actions/runs/15954167507/job/44998244253?pr=3061 https://github.com/apache/arrow-adbc/actions/runs/15954167507/job/44998244254?pr=3061  | 
    
| 
           This works for me!  | 
    
…R CMD check for Go-based drivers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this properly! This particular CRAN rule is I think one of their worst additions to date (actively making it harder to package useful libraries to R 🙁 ). Just one optional request to document why we need the extra makefile target but in general this looks great!
| purify: $(SHLIB) | ||
| rm -Rf "$(CURDIR)/go/libadbc_driver_snowflake.a" | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a note (here and to the other files) about why we have to do this? (Mostly so I don't forget the next time I look here 😬 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about 9c63bfa?
| 
           This is a side note, but I noticed that only adbcsnowflake does not have Makevars.win.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
          
 Yes, we can. The Go drivers don't build without the UCRT R versions (although I think Snowflake did at one point, hence the file).  | 
    
Fix #3059 (Supersede #2708)