Skip to content
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

Allow cross platform installation by custom libc flag #2692

Merged
merged 1 commit into from
May 10, 2021

Conversation

xemle
Copy link
Contributor

@xemle xemle commented May 4, 2021

I would like to create cross platform bundles including sharp prebuilds, e.g. installing sharp for darwin-x64 on a linux-arm64 machine. For that reason the libc checks needs to be disabled - or the installation needs to be forced.

I am aware that this flag is kind of I-know-what-I-am-doing - I am open for discussions

And again: Thank you for this great library :-)

@lovell
Copy link
Owner

lovell commented May 4, 2021

Would adding support for an optional --libc flag passed via npm provide something similar?

$ npm install --platform=linux --libc=musl ...

This would arrive via process.env.npm_config_libc and, if set, could be compared against detectLibc.family before performing the version check.

@xemle
Copy link
Contributor Author

xemle commented May 4, 2021

I could not follow your idea completely. You mean something like

    if ((process.env.npm_config_libc || detectLibc.family) === detectLibc.GLIBC && detectLibc.version) {
      if (semverLessThan(`${detectLibc.version}.0`, `${minimumGlibcVersionByArch[arch]}.0`)) {
        throw new Error(`Use with glibc ${detectLibc.version} requires manual installation of libvips >= ${minimumLibvipsVersion}`);
      }
    }

?

Would it make more sense to provide a patch to detect-libc module to allow manual overwrites the libc family by --libc and version by --libc-version?

@lovell
Copy link
Owner

lovell commented May 4, 2021

That code sample looks like it does what I was thinking, yes. Does that solve your problem? I'd prefer to address this in sharp rather than detect-libc.

@xemle xemle force-pushed the feature/disable-libc-check branch from 3f2e222 to 3e08ba7 Compare May 4, 2021 20:51
@xemle
Copy link
Contributor Author

xemle commented May 4, 2021

OK. I updated my PR. Type of --libc is checked against undefined

So npm --platform=linux --arch=arm64 --libc= run install is possible on my linux-x64 machine. Awesome.

@xemle xemle force-pushed the feature/disable-libc-check branch 2 times, most recently from b3b8144 to 73a248a Compare May 5, 2021 05:23
@xemle
Copy link
Contributor Author

xemle commented May 5, 2021

I've fixed the syntax and thus CI builds and updated the doc with hint of cross platform builds by --libc flag.

Feel free to rephrase it.

@lovell
Copy link
Owner

lovell commented May 5, 2021

Thanks for the updates. I've just had a thought - what method/flags are you currently using to "cross" install at the moment?

I ask as, having said e.g. --libc=musl would be suitable to detect this, I now realise that people might already be using e.g. --platform=linuxmusl instead. If so, perhaps that would be a better check. Apologies for not thinking of this earlier.

@xemle
Copy link
Contributor Author

xemle commented May 5, 2021

As I've written my usecase is to install linux-x64 from a linux-arm64 host and vise versa. The libc detection check is only triggered on linux hosts and I am looking for an I-know-what-I-am-doing thing.

AFAIK setting the plattform to linuxmusl would not help here.

Further feedback and suggestions are wellcome ;-)

@xemle xemle changed the title Allow cross platform installation by disable libc check flag Allow cross platform installation by custom libc flag May 6, 2021
@xemle
Copy link
Contributor Author

xemle commented May 7, 2021

After some thought the semantic of the --libc flag is misleading for me. At the end with the flag I can disable the libc detection on the host.

I propose the the initial change of a --disable-libc-detection flag or a --no-libc-check. It is than in the responsible of the caller to ensure the compatibility of the libc.

@lovell
Copy link
Owner

lovell commented May 8, 2021

Yes, I've been thinking about this more too and I agree with you, let's keep this simple like you initially suggested.

In terms of naming, I can't remember/find what you first proposed, but we could follow the existing "I know what I'm doing" SHARP_IGNORE_GLOBAL_LIBVIPS environment variable pattern with something like SHARP_IGNORE_LIBC.

@xemle
Copy link
Contributor Author

xemle commented May 8, 2021

Thank you for your reply. I'm fine with SHARP_IGNORE_LIBC.

What about with a more general name of SHARP_INSTALL_FORCE?

@lovell
Copy link
Owner

lovell commented May 8, 2021

OK, SHARP_INSTALL_FORCE sounds good.

Perhaps we should still perform the checks but log a warning rather than throw an Error? If we go for this then the supportedNodeVersion check can be altered likewise.

@xemle xemle force-pushed the feature/disable-libc-check branch from 73a248a to f872c85 Compare May 8, 2021 12:58
@xemle
Copy link
Contributor Author

xemle commented May 8, 2021

I've updated the code and introduced --sharp-install-force flag or SHARP_INSTALL_FORCE env variable. It handles also the node engine like the libc detection. On forced installation the error message will be only logged with a continue information. Otherwise an error will be thrown as before.

Works and feels good so far ;-) If I should change/rename/document anything, please let me know.

@xemle xemle force-pushed the feature/disable-libc-check branch from f872c85 to e31ffa7 Compare May 8, 2021 13:03
@xemle
Copy link
Contributor Author

xemle commented May 8, 2021

Another question: Why is the CI job for FreeBSD 13.0 failing?

../src/common.cc:24:10: fatal error: 'vips/vips8' file not found
#include <vips/vips8>
         ^~~~~~~~~~~~
1 error generated.

Is this error triggered by this PR? Or is this a general issue?

@lovell lovell merged commit 476448b into lovell:master May 10, 2021
@lovell lovell added this to the v0.28.2 milestone May 10, 2021
@lovell
Copy link
Owner

lovell commented May 10, 2021

Thank you very much for this, it will be included in v0.28.2.

(The FreeBSD CI is currently failing due to the out-of-date vips package.)

@xemle
Copy link
Contributor Author

xemle commented May 11, 2021

Thank you very much for accepting my PR. Awesome

@xemle xemle deleted the feature/disable-libc-check branch May 11, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants