add PROG_IS_C_ONLY variables for C-only programs

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla57

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8883976 [details] [diff] [review]
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.
Attachment #8883976 - Flags: review?(cmanchester)
(Assignee)

Comment 2

2 years ago
Created attachment 8883977 [details] [diff] [review]
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.)
Attachment #8883977 - Flags: review?(cmanchester)
(Assignee)

Updated

2 years ago
Blocks: 1163171
Why not merge both and have TARGET_IS_C_ONLY?
(Assignee)

Comment 4

2 years ago
(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)
(Assignee)

Comment 7

a year ago
Created attachment 8891032 [details] [diff] [review]
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.
Attachment #8891032 - Flags: review?(cmanchester)
(Assignee)

Updated

a year ago
Attachment #8883976 - Attachment is obsolete: true
(Assignee)

Comment 8

a year ago
Created attachment 8891033 [details] [diff] [review]
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.
Attachment #8891033 - Flags: review?(cmanchester)
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 10

a year ago
(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)
(Assignee)

Comment 12

a year ago
Created attachment 8897894 [details] [diff] [review]
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.
Attachment #8897894 - Flags: review?(cmanchester)
(Assignee)

Updated

a year ago
Attachment #8891032 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
Created attachment 8897895 [details] [diff] [review]
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.
Attachment #8897895 - Flags: review?(cmanchester)
(Assignee)

Updated

a year ago
Attachment #8891033 - Attachment is obsolete: true
Attachment #8891033 - Flags: review?(cmanchester)
Attachment #8897894 - Flags: review?(cmanchester) → review+
Attachment #8897895 - Flags: review?(cmanchester) → review+

Comment 14

a year ago
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

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de4a2cd6e18c
https://hg.mozilla.org/mozilla-central/rev/2dc6fcbd991b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

10 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.