770717 - Pymake: support conditional native commands

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: first cut patch

What we're seeing a lot in the Mozilla build system is native commands (e.g. $(INSTALL) for nsinstall) that also need to have non-native fallbacks if invoked within a shell. I think the easiest way out here is supporting a way to specify both a native command and a non-native fallback for the same variable.

Here's a first cut at this. This has a bunch of redundant calls and so can probably be optimized a fair bit. Is it worth it, though? I'm putting this up for feedback.

Comment 1

[Gregory Szorc [:gps]]

Comment on attachment 638897 [details] [diff] [review]
first cut patch

Review of attachment 638897 [details] [diff] [review]:
-----------------------------------------------------------------

I like the idea, but the implementation has issues.

bsmedberg and/or others should weigh in on the double execution bit.

Also, perhaps this patch would be a good time to update the README to include a blurb about how native commands work...

::: pymake/data.py
@@ +1236,5 @@
> +        # To support conditional native commands, first try evaluating as a
> +        # native command.
> +        vnative = Variables(parent=v)
> +        vnative.set('.PYMAKE_NATIVE', Variables.FLAVOR_SIMPLE,
> +                    Variables.SOURCE_IMPLICIT, '1')

I think a whole new Variables instance is overkill. There's little to no harm in having .PYMAKE_NATIVE always defined and changing the value between an empty string (false with a $(if)) and a non-empty string when it's needed.

I'm pretty sure variables with leading periods are considered reserved, so you shouldn't have to worry about others defining this variable.

@@ +1250,5 @@
> +                forcenonnative = True
> +                break
> +        if forcenonnative:
> +            # Restart with .PYMAKE_NATIVE unset for this command
> +            cstring = c.resolvestr(makefile, v)

A potential problem here is we have 2 calls to resolvestr(). If the Expansion contains any $(shell), $(call), or similar functions with side effects, that could be bad.

You can make the argument that this would be a poorly written make file. I would tend to agree. But, this doesn't change that the double execution might be disastrous. I think any solution involving double execution should be an automatic r-.

That being said, the whole Python native command thing is all a giant hack, so maybe it isn't such a big deal. I defer ultimate judgement.

Another problem is this relies on the command implementing $(if $(.PYMAKE_NATIVE)...). If it doesn't and the variable always evaluates to a native command, we are back where we started. You should probably factor out this logic into a method and do an additional test once the supposed non-native evaluation is performed. If there is still a native command, you should probably abort execution.

::: pymake/process.py
@@ +86,5 @@
>                       justprint=justprint)
>          return
>  
> +    argv, _ = clinetoargv(cline)
> +    argv = doglobbing(argv, cwd)

Where did these come from? Is Splinter being funky?

Comment 2

[Rain [:rain1] (inactive)]

Yes, the side-effect point is very good and completely invalidates this approach. I think we'll need to add more syntax.

Comment 3

[Rain [:rain1] (inactive)]

I'm going to put this on hold to see how badly we still need it after bug 370750 and bug 769378.

Comment 4

[Gregory Szorc [:gps]]

Another idea: invent a new function that resolves to whatever is appropriate:

$(command $(NSINSTALL), nsinstall nsinstall)

Although, immediate expansion could have undesirable consequences :/

Comment 5

[Chris Cooper [:coop] (he/him)]

Mass close of pymake-related bugs.