Closed Bug 1378830 Opened 2 years ago Closed 2 years ago

add PROG_IS_C_ONLY variables for C-only programs

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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.
Attachment #8883976 - Flags: review?(cmanchester)
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.)
Attachment #8883977 - Flags: review?(cmanchester)
Why not merge both and have TARGET_IS_C_ONLY?
(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 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?
Attachment #8883976 - Flags: review?(cmanchester)
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.
Attachment #8883977 - Flags: review?(cmanchester)
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.
Attachment #8891032 - Flags: review?(cmanchester)
Attachment #8883976 - Attachment is obsolete: true
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.
Attachment #8891033 - Flags: review?(cmanchester)
Attachment #8883977 - Attachment is obsolete: true
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?
Attachment #8891032 - Flags: review?(cmanchester)
(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?
Flags: needinfo?(cmanchester)
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?
Flags: needinfo?(cmanchester)
I'm still not sure I completely understand the objection, but here's a patch
that does what you want, I think.
Attachment #8897894 - Flags: review?(cmanchester)
Attachment #8891032 - Attachment is obsolete: true
Minor adjustment to define an empty BIN_SUFFIX for testing now that part 1
requires that.
Attachment #8897895 - Flags: review?(cmanchester)
Attachment #8891033 - Attachment is obsolete: true
Attachment #8891033 - Flags: review?(cmanchester)
Attachment #8897894 - Flags: review?(cmanchester) → review+
Attachment #8897895 - Flags: review?(cmanchester) → review+
Pushed by nfroyd@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/de4a2cd6e18c
https://hg.mozilla.org/mozilla-central/rev/2dc6fcbd991b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.