[wip] Allow overriding target platform when cross-compiling

[ralphtheninja]

Supersedes #20

[ralphtheninja]

No need to release new version once merged.

[ralphtheninja]

@ahdinosaur I reverted your changes and now I'm only modifying the initial opts.platform and don't pass that along to run() etc. The reason for this is that when cross-compiling we only care about the resulting builds folder here:

prebuildify/index.js

Line 33 in 1125e68

builds: path.join(opts.cwd, 'prebuilds', opts.platform + '-' + opts.arch),

And we do not care about it in e.g. run() here:

prebuildify/index.js

Line 109 in 1125e68

var shell = os.platform() === 'android' ? 'sh' : undefined

nor here:

prebuildify/index.js

Line 138 in 1125e68

var nodeGypBin = os.platform() === 'win32' ? 'node-gyp.cmd' : 'node-gyp'

The last two cases should always use os.platform() since they care about which platform we're building on and not the target platform (that is to say when the target platform is the same as the platform we're building on). This is different when we are cross-compiling for e.g. android (the target platform) but doing it on Debian.

Makes sense?

We might need to distinguish this in the code though for less confusion.

[ralphtheninja]

Setting wip status once again. Need to test and play around with this for a while.

[vweevers]

We might need to distinguish this in the code though for less confusion.

Maybe rename opts.platform to e.g. opts.targetPlatform?

[ralphtheninja]

Maybe rename opts.platform to e.g. opts.targetPlatform?

At first I thought yes, but we're doing xtend() with incoming opts so I'd like to refrain from renaming it for now.

[vweevers]

Wouldn't #25 mean a semver-major anyway? If not, we could also do:

opts = xtend({
    arch: process.env.ARCH || os.arch(),
    targetPlatform: opts.platform || process.env.TARGET_PLATFORM || os.platform(),
    cwd: '.',
    targets: []
  }, opts)

[ralphtheninja]

Wouldn't #25 mean a semver-major anyway? If not, we could also do:

Yeah that's true!

[ralphtheninja]

Travis borked for Windows 10. Restarting build.

[vweevers]

Sorry for the turnaround, but I just noticed opts.arch !== os.arch() in:

prebuildify/index.js

Lines 37 to 39 in 8529e6d

	if (opts.arch === 'ia32' && opts.targetPlatform === 'linux' && opts.arch !== os.arch()) {
	opts.env.CFLAGS = '-m32'
	}

Which implies opts.arch also means "target arch", in which case the rename of opts.platform to opts.targetPlatform makes less sense unless we also rename opts.arch...

[ralphtheninja]

Which implies opts.arch also means "target arch", in which case the rename of opts.platform to opts.targetPlatform makes less sense unless we also rename opts.arch...

😆

Agreed. I'd like to keep it as opts.arch for now though to avoid this PR exploding into something more than just overriding platform.

[vweevers]

I'd like to keep it as opts.arch for now though to avoid this PR exploding into something more than just overriding platform.

👍 (I meant that for now it'd be better to revert renaming platform, rather than also renaming arch)

[ralphtheninja]

+1 (I meant that for now it'd be better to revert renaming platform, rather than also renaming arch)

Aaah ok. Sure!

[ahdinosaur]

hi everyone, sorry i've been away at a festival for over a week, thanks for reaching out @ralphtheninja. 😺

i agree the opts.platform vs os.platform() vs process.env.TARGET_PLATFORM is hella confusing, same for opts.arch vs os.arch() vs process.env.ARCH. i personally prefer verbose and explicit over terse and implicit, but am happy at least if everything is clearly documented.

so to loop back my understanding:

• there are two types of platforms: the host platform (the platform we are building on) and the target platform (the platform we are building for). same for arches: host arch and target arch.
• we need to be able to override the target platform via opts or process.env, for cross-compiling
• the host platform is always able to be retrieved with os.platform()

❤️

[vweevers]

A first stab at documenting this:

Option	Input	Subprocess env	node-gyp args
target arch	opts.arch or ARCH or os.arch()	ARCH, PREBUILD_ARCH	--target_arch
target platform	opts.platform or TARGET_PLATFORM or os.platform()	PREBUILD_PLATFORM	-
enable strip	opts.strip or false	n/a	n/a
strip bin	STRIP or 'strip'	-	n/a
strip args	os.platform() == 'darwin' ? '-Sx' : '--strip-all'	n/a	n/a
node gyp bin	PREBUILD_NODE_GYP or 'node-gyp(.cmd)'	-	n/a
shell	os.platform() == 'android' ? 'sh' : undefined	-	n/a

There's no easy (non-breaking) way to clean this up, but now would be the time. Knowing that:

1. We cannot use PLATFORM for Windows-compat reasons
2. It may be useful to have a normalized env for subprocesses (like pre/post hooks).
3. There seems to be consensus that a TARGET_ prefix makes names more intuitive
4. But we don't "own" TARGET_. To prevent further conflicts PREBUILD_ makes sense.
5. That way we can confidently add more env variables, both as input and output.

I propose (changes bold) (I'm not sure what to do with strip args):

Option	Input	Subprocess env	node-gyp args
target arch	opts.arch or PREBUILD_ARCH or os.arch()	ARCH, PREBUILD_ARCH	--target_arch
target platform	opts.platform or PREBUILD_PLATFORM or os.platform()	PREBUILD_PLATFORM	-
enable strip	opts.strip or PREBUILD_STRIP or false	PREBUILD_STRIP	n/a
strip bin	opts['strip-bin'] or PREBUILD_STRIP_BIN or 'strip'	PREBUILD_STRIP_BIN	n/a
strip args	os.platform() == 'darwin' ? '-Sx' : '--strip-all'	n/a	n/a
node gyp bin	opts['node-gyp'] or PREBUILD_NODE_GYP or 'node-gyp(.cmd)'	PREBUILD_NODE_GYP	n/a
shell	opts.shell or PREBUILD_SHELL or os.platform() == 'android' ? 'sh' : undefined	PREBUILD_SHELL	n/a

[mafintosh]

@vweevers looks really good and I agree with your prefix. For simplicity/completeness I would name strip simply PREBUILD_STRIP with same semantics as above. Then there is no special case for that one

[mafintosh]

Do we support setting strip args from the outside?

[vweevers]

For simplicity/completeness I would name strip simply PREBUILD_STRIP

Table updated.

Do we support setting strip args from the outside?

We could start with process.env.PREBUILD_STRIP_ARGS.split(' ') which would be sufficient until someone needs exotic arguments like --foo "arg with spaces" - if that is even a thing. For comparison, in prebuild we don't support custom args either. Let's leave it out for now?

[vweevers]

I'll open a new PR (but please keep this override-platform branch, leveldown master depends on it atm).