Skip to content

fix(c/driver_manager): fix expected ; for musl arch #3105

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.

Sign up for GitHub

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

Jump to bottom
Merged
lidavidm merged 1 commit into from
Jul 7, 2025
Merged

fix(c/driver_manager): fix expected ; for musl arch #3105

lidavidm merged 1 commit into from
Jul 7, 2025

Conversation

@eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Jul 7, 2025 •
edited
Loading

I noticed that the WASM build for the ADBC 19 release on R-multiverse is failing.

I was able to reproduce the error locally using an Alpine-based Docker image, as shown below:

docker run --rm -it ghcr.io/r-hub/r-minimal/r-minimal:latest bash

Then, execute the following command on the container:

installr -c apache/arrow-adbc/r/adbcdrivermanager@apache-arrow-adbc-19

The error will be as follows:

In file included from c/driver_manager/adbc_driver_manager.cc:39:
c/driver_manager/current_arch.h:71: warning: "__MUSL__" redefined
   71 | #define __MUSL__
      |
<command-line>: note: this is the location of the previous definition
c/driver_manager/current_arch.h: In function 'const std::string& adbc::CurrentArch()':
c/driver_manager/current_arch.h:84:3: error: expected ',' or ';' before 'static'
   84 |   static const std::string result = platform + "_" + arch + target;
      |   ^~~~~~
c/driver_manager/current_arch.h:85:10: error: 'result' was not declared in this scope
   85 |   return result;
      |          ^~~~~~
make: *** [/usr/local/lib/R/etc/Makeconf:209: c/driver_manager/adbc_driver_manager.o] Error 1
ERROR: compilation failed for package ‘adbcdrivermanager’

In the future it may be worthwhile to have musl builds on CI.

lidavidm
lidavidm approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks for catching this!

@eitsupi eitsupi marked this pull request as ready for review July 7, 2025 11:15
@github-actions github-actions bot added this to the ADBC Libraries 20 milestone Jul 7, 2025
@eitsupi
Copy link
Contributor Author

eitsupi commented Jul 7, 2025

I had to leave my desk so I didn't fully write down the description, but I did complete it.
I am not familiar with the CI of this repository, but I am surprised clang_tidy did not catch this.

lidavidm
lidavidm approved these changes Jul 7, 2025
View reviewed changes
Copy link
Member

@lidavidm lidavidm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, we should consider a MUSL build too. Want to file a new issue? (We could just have a Docker Compose job that builds the source and run it as a nightly test, like we already do to test newest Clang/GCC.)

@lidavidm
Copy link
Member

lidavidm commented Jul 7, 2025

Ignoring the Dev PR check since it's incorrectly reading the @ in the PR body as trying to ping a user

@lidavidm lidavidm merged commit 218708c into apache:main Jul 7, 2025
69 of 72 checks passed
@eitsupi eitsupi mentioned this pull request Jul 7, 2025
Closed
@eitsupi
Copy link
Contributor Author

eitsupi commented Jul 7, 2025

We could just have a Docker Compose job that builds the source and run it as a nightly test, like we already do to test newest Clang/GCC.

Thanks, sounds good. I've opened #3107

@eitsupi eitsupi deleted the fix-semicolon branch July 7, 2025 15:30
@bastistician
Copy link

bastistician commented Jul 18, 2025

FWIW, I am now seeing this compilation error in the new CRAN release of package 'adbcdrivermanager' (0.19.0). Checks on Alpine aren't part of the submission checks yet, so this slipped through. A patch version would be nice.

@eitsupi
Copy link
Contributor Author

eitsupi commented Jul 18, 2025

@bastistician Due to the amount of effort involved in the Apache release process in general, I think it is unlikely that a patched version will be released for this. In that case, the next release will probably be more than a month away.

But @paleolimbot could submit a patched version to CRAN.
(I'm not sure how many users want to install adbcdrivermanager from CRAN on musl Linux, so I'm not sure if it's worth the effort. Just ping @paleolimbot)

@paleolimbot
Copy link
Member

paleolimbot commented Jul 18, 2025

I can cherry pick this into a patch no problem!

@paleolimbot
Copy link
Member

paleolimbot commented Jul 19, 2025 •
edited
Loading

I did the pick and will push to CRAN shortly. I get:

In file included from c/driver_manager/adbc_driver_manager.cc:39:
c/driver_manager/current_arch.h:71:9: warning: "__MUSL__" redefined
   71 | #define __MUSL__
      |         ^~~~~~~~

(i.e., this should probably also check if defined(__MUSL__) since it appears to be defined as part of the flags in some places, such as the default flags for building R packages on alpine)

@eitsupi
Copy link
Contributor Author

eitsupi commented Jul 20, 2025

@paleolimbot Thanks!
Sorry if my understanding is wrong, but isn't that line already included in version 0.19.0 that's already released on CRAN?
https://github.com/cran/adbcdrivermanager/blob/b5a149e907d0b57c667eff44dd61241d6bb626f4/src/c/driver_manager/current_arch.h#L71

@serramatutu serramatutu mentioned this pull request Aug 12, 2025
Closed
@eitsupi eitsupi mentioned this pull request Dec 20, 2025
Merged
lidavidm pushed a commit that referenced this pull request Dec 21, 2025
@eitsupi
ci: add a nightly build job for musl linux (alpine) (#3434)
9fb06be 
Close #3107

I've prioritized testing the driver manager and added a minimal build,
which we hope will catch musl-specific bugs like #3105.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidm lidavidm lidavidm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ADBC Libraries 20

Development

Successfully merging this pull request may close these issues.

4 participants

@eitsupi @lidavidm @bastistician @paleolimbot