Closed Bug 1491901 Opened 6 years ago Closed 6 years ago

move MK*SHLIB to moz.configure

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox-esr60 fixed, firefox64 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox64 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files)

move MK*SHLIB to moz.configure
(deleted), patch
ted
: review+
Details | Diff | Splinter Review
move MK*SHLIB to moz.configure (esr60)
(deleted), patch
jcristau
: approval-mozilla-esr60+
Details | Diff | Splinter Review
It's nicer to have all that logic in one place, and to be able to common up the Unix-y flags setting. The Makefile constructs in string values is gross, but it's no worse than we had before.
Attached patch move MK*SHLIB to moz.configure (deleted) — — Splinter Review
It's not the nicest patch ever, but I like pulling everything together in one place.
Attachment #9009678 - Flags: review?(core-build-config-reviews)
Comment on attachment 9009678 [details] [diff] [review] move MK*SHLIB to moz.configure Review of attachment 9009678 [details] [diff] [review]: ----------------------------------------------------------------- This is definitely not fantastic but I guess it's better than having it in old-configure. I'm not sure what the ideal state would be here--maybe setting the platform-specific bits in moz.configure, then assembling them as a `COMPUTED_FLAGS`-style thing in the emitter?
Attachment #9009678 - Flags: review?(core-build-config-reviews) → review+
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/273221b765bf move MK*SHLIB to moz.configure; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/273221b765bf
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Will this be taken to esr60? (Register my vote for 'yes' - trying to figure out if I need to rewrite a patch from Bug 1475562)
Flags: needinfo?(mozilla)
Nathan; do you think this should/could be backported to esr60? I gave it a shot here and it seems to be okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e4f82ab8d3ff7fed137ab0e2762745d87f11c89 (not counting Bug 1516642)
Flags: needinfo?(nfroyd)
(In reply to Tom Ritter [:tjr] from comment #6) > Nathan; do you think this should/could be backported to esr60? If this is needed for other patches, I don't see why not. The porting shouldn't be difficult.
Flags: needinfo?(nfroyd)

This is a question for somebody from the build side.

Flags: needinfo?(mozilla)

Comment on attachment 9035854 [details] [diff] [review]
move MK*SHLIB to moz.configure (esr60)

This is one of series of patches I am requesting uplift to esr60. Please don't uplift any if the entire series won't go. The whole series will need to go in one push.

This try run (applied on tip-of-esr60 as of an hour ago; and beginning with 'Bug 1491901 - move MK*SHLIB to moz.configure') represents the patch series. It must be applied in that order: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0659d6e265f3b624ad6fbac0c9cd7ce246094596 (If this try run doesn't complete successfully, I will investigate and figure out why)

The uplift request form is the same for all of the patch series.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tor is in the middle of switching their esr60 builds from mingw-gcc to mingw-clang. This will allow them to use Stylo, and migrate off the older, harder-to-suport platform. We want to get the esr60 tree in parity with -central for the mingw-clang build for them. Otherwise it's going to be very confusing for them to compare their development work with ours.

This patch series will enable us to uplift a component of the mingw-clang build from -central (specifically pdb generation) and perform sanity checks of the resulting xul.dll (NSModule checks primarily).

User impact if declined: the mingw-clang build (and toolchain) for -central and -esr60 will be out of sync. This will make it confusing for them while they develop their toolchain and make the switch to it (in https://trac.torproject.org/projects/tor/ticket/28716 ). It will be much harder for us to track down compilation errors and differences between their esr60 and our central branch. (And we know we're going to track down differences.)

Risk to taking this patch: Medium.

Why? It's an 11 patch series. However, its saving grace is that it doesn't make any changes to Firefox, it only refactors how certain build commands are provided during the build process. (Refactoring them from Makefiles and piped shell into more maintainable and comprehensible python. The only change for Firefox itself is in Bug 1481633 and it only affects mingw-clang.

Alternatives? It might be possible to achieve the toolchain updates we want (pdb generation) without backporting this series; but it's going to be really ugly doing so. I'm not sure if I will be able to do it in time for Tor before I go on leave.

Attachment #9035854 - Flags: approval-mozilla-esr60?

Comment on attachment 9035854 [details] [diff] [review]
move MK*SHLIB to moz.configure (esr60)

approved for 60.5esr

Attachment #9035854 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: