1378830 - add PROG_IS_C_ONLY variables for C-only programs

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nathan Froyd [:froydnj]]

We already have LIB_IS_C_ONLY to denote whether libraries can be linked solely
with the C compiler.  We should also have equivalent things for programs.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS

Similar to the existing LIB_IS_C_ONLY variable, these variables indicate
that the program in question has only C sources and so can be linked by
the C compiler rather than the C++ compiler.  We need to add a little
more information to BaseProgram so we can avoid emitting periods into
Makefile variables.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add tests for {LIB,PROG}_IS_C_ONLY

Adding PROG_IS_C_ONLY seems like a good point to add tests, and once we
have tests for that, adding tests for the existing library support is
not too difficult.

(I know that tests should really be added with the patch that introduces new
features, but the tests here were lasrge enough that I felt it was worthwhile
to split them out into a separate commit.)

Comment 3

[Mike Hommey [:glandium]]

Why not merge both and have TARGET_IS_C_ONLY?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #3)
> Why not merge both and have TARGET_IS_C_ONLY?

What if SIMPLE_PROGRAMS contains things that are compiled from C sources and things compiled from C++ source?  emitter.py has some comments about this scenario:

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#836

I'm not sure if it's actually a problem in practice, though.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8883976 [details] [diff] [review]
part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS

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

::: python/mozbuild/mozbuild/frontend/data.py
@@ +389,4 @@
>              program += bin_suffix
> +        else:
> +            base_program = program[:len(bin_suffix)]
> +        self.base_program = base_program

It appears we're depending on PROG_IS_C_ONLY only being interesting on platforms where bin_suffix == "". In other words, we're setting base_program for programs on windows, but if we ever looked at PROG_IS_C_ONLY_$@ there it wouldn't match what we set in recursivemake.py, because "$@" would include ".exe". Can we avoid setting PROG_IS_C_ONLY variables on platforms it isn't significant, or at least add a comment about why this works?

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8883977 [details] [diff] [review]
part 2 - add tests for {LIB,PROG}_IS_C_ONLY

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

Will look at this again with the first patch in case updates are needed.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS

I thought that everything compiled with clang with these patches, but some
investigation this week showed me I was wrong, and it was all related to
problems with program suffixes.  Thanks for catching that.

The logic in BaseProgram should be a little more understandable now.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add tests for {LIB,PROG}_IS_C_ONLY

No changes to the tests, just asking for re-review.  I thought about trying to
extend things so that you could pass in a custom config (environment variables,
configure settings, etc.) for a given backend test directory, but wasn't sure
if it was worth it.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8891032 [details] [diff] [review]
part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS

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

Sorry if I'm misunderstanding this patch, but I don't think my original comment was actually addressed here.

Isn't the "base_program" stuff just letting us emit pointless PROG_IS_C_ONLY_* variables on platforms where we don't use them?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Chris Manchester (:chmanchester) from comment #9)
> Sorry if I'm misunderstanding this patch, but I don't think my original
> comment was actually addressed here.
> 
> Isn't the "base_program" stuff just letting us emit pointless
> PROG_IS_C_ONLY_* variables on platforms where we don't use them?

Oh, I see what you're saying.  We could modify the Makefile to strip suffixes before consulting PROG_IS_C_ONLY_*.

I guess we could try to just not emit PROG_IS_C_ONLY_* on particular platforms, but I don't know why this particular variable should be considered special, compared to LIB_IS_C_ONLY (which also only matters on some platforms) or other variables.  Can you explain the rationale here?

Comment 11

[Chris Manchester (limited bugmail, email directly)]

This is more convoluted than LIB_IS_C_ONLY because we're adding additional logic to sanitize variables that never get used.

I suppose it's not really doing any harm, but is it really a hassle to just emit these variables where we need them?

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS

I'm still not sure I completely understand the objection, but here's a patch
that does what you want, I think.

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add tests for {LIB,PROG}_IS_C_ONLY

Minor adjustment to define an empty BIN_SUFFIX for testing now that part 1
requires that.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4a2cd6e18c
part 1 - define PROG_IS_C_ONLY variables for PROGRAM and SIMPLE_PROGRAMS; r=chmanchester
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc6fcbd991b
part 2 - add tests for {LIB,PROG}_IS_C_ONLY; r=chmanchester

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de4a2cd6e18c
https://hg.mozilla.org/mozilla-central/rev/2dc6fcbd991b