-
Notifications
You must be signed in to change notification settings - Fork 87
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
Use system copy of libuv if found #368
Conversation
The builds don't print the package installation output, so you can't tell whether system or bundled libuv are being picked up. I made rstudio/shiny-workflows#17 to try to add that to the GHA workflow. |
@schloerke I'm happy to take this on if you'd like |
@nealrichardson yes, please! If you need to make changes to rstudio/shiny-workflows, please feel free! |
386a9cb
to
a9ca6d7
Compare
In the regular CI, the ubuntu images do not have libuv installed so they all still bundle. The macOS job does appear to have it installed by homebrew, and the configure script picks it up and uses it fine. |
For now, I've set the minimum version to 1.43, which is what we vendored. Looking at https://github.com/rstudio/httpuv/commits/main/src/libuv/include/uv/version.h, there have been few updates, and only one mentions a reason for the upgrade. @wch do you happen to recall if there were reasons (bugfixes, new features) that motivated the updates in the past? I'll check what ubuntu and centos et al. have in their repositories and see if they work in the test suite, though that might not be enough confidence that they're good to use if they're older than our version. |
@nealrichardson Typically we have updated httpuv not for any specific reason, but just to keep up to date with bug fixes. |
I learned two new things about this topic yesterday that I don't fully understand. First, from CRAN Policies (emphasis mine):
As written, when the local libuv is found, it's using the dynamically linked version. To get the statically linked version, it would be Second, following the link in the previous quote, you get this paragraph:
I don't know much about this new build system, but it certainly seems like MXE is supposed to help us with libuv somehow on Windows? Maybe it's just saying that on the Windows CRAN build machines, the static version of this library will be available?? |
I believe it's There is no "recipe" for libuv currently on https://github.com/R-macos/recipes, so I don't think the CRAN builders will have it. But if/when it does exist, we can pick it up. I don't think getting libuv on there will be a requirement for resubmission, I would guess that BDR will only look for the configure script to see that we are looking for system packages. But if we have to go that route, we can, and I'm hoping it's simple enough, it doesn't look like libuv requires complex configuration.
libuv is on MXE, but it's an older version, and IIUC we have a few patches we apply to our copy that haven't been upstreamed (yet). So I think that if we're asked about this (I doubt we will but you never know), we say that the version there is not adequate, and we'll work to get it upgraded for the next time we submit httpuv. |
725ec43
to
41da7ee
Compare
6c6d532
to
d58d5de
Compare
@jcheng5 I think this is done if you want to take a look (I can't request your review since technically it is your PR). |
OK. I think you're probably right, but please take a look at this issue if you haven't seen it already: libuv/libuv#2988 Edit: Actually, with what they're describing in that issue, the build would fail and ours is passing, so I withdraw the concern. |
# This one should test for bundling log message | ||
- {install: true, use: true} | ||
# This one should test for failure | ||
- {install: false, use: false} |
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.
Note to self. This is all of the possible combinations and their expected results:
Install | Use | expected result | |
---|---|---|---|
1 | true | unset | Uses system |
2 | false | unset | Uses bundled |
3 | true | true | Uses bundled |
4 | false | true | Uses bundled |
5 | true | false | Uses system |
6 | false | false | Installation fails |
(I think you did a good job of narrowing it down to 3 configs)
LGTM! I'm glad you volunteered to take this on, I am not currently equipped for this level of GHA sophistication. |
I'll double-check this locally. Brew installs both libuv.a and libuv.dylib. If it's working correctly, I should be able to brew uninstall libuv after installing the R package, and the R package should still work. In practice it should be irrelevant for the CRAN builders. |
Adds a configure script that looks for libuv on the system that is version 1.43 (what we bundle) or newer. On macOS, static linking is preferred. If the library isn't found, installation falls backed to the bundled copy, as before. There is a new environment variable,
USE_BUNDLED_LIBUV
, that can be set to override this behavior:true
means it won't look for system libuv,false
means it won't build the bundled library and will fail if the system library isn't found.Homebrew and Ubuntu 22.04 have new enough versions of libuv that could be installed. Our regular CI picks up the brew package in the macos-latest job, and I added a new workflow that tests the ubuntu package and the various combinations of the
USE_BUNDLED_LIBUV
variable.