Apply 2-character indents to tooltool manifests

RESOLVED FIXED in Firefox 55

Status

RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

unspecified
mozilla55

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Our tooltool manifests are written with newlines but without indentation, which makes them harder to read. Modern code editors should be able to maintain json indentation, so I don't think there's a reason to keep them as-is.
This also smooths bumping the rust toolchains; currently manifests aren't updated except for the Android sdk 'versions' array. It would be nice to not have to special-case those. Our in-tree copy of tooltool was updated to produce indented manifests in bug 1325225.
Assignee: nobody → giles
Comment hidden (mozreview-request)
Missed browser/config/tooltool-manifests/linux64/clang.manifest.centos6
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8846770 [details]
Bug 1346897 - Indent tooltool manifests.

https://reviewboard.mozilla.org/r/119774/#review122146

I also see a few others by grepping for '^"digest":'. Most are b2g related, but ./js/src/devtools/rootAnalysis/build/gcc.manifest and ./testing/config/tooltool-manifests/androidarm_6_0/mach-emulator.manifest stick out. (I'm not sure if those are actually used anywhere).
Attachment #8846770 - Flags: review?(mshal) → review+
I have some patches I was fiddling with locally that add better support to tooltool for reading/writing manifests, so you could modify and round trip them (since I've had such a pain doing this in the past). I think I let those patches fall by the wayside because I'm hoping glandium will fix bug 1313111 soon...
rillian asked for clarification in IRC, and I realize the relevance may not have been clear! I have a patch here that gives tooltool.py the ability to read and write manifests without extraneous diffs, and then I have another patch in which I ran that on every manifest in the tree, and it does produce a bunch of diffs, mostly because of the lack of indentation of our in-tree manifests.

This is what that diff looks like:
https://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/415252474c01
Ok, that sounds like the same inconsistent-indent problem I was trying to fix. I suppose while people are hand-editing these files we either need a lint to maintain the formatting or a merge tool that maintains the indentation it finds.
(In reply to Michael Shal [:mshal] from comment #5)

> I also see a few others by grepping for '^"digest":'. Most are b2g related,
> but ./js/src/devtools/rootAnalysis/build/gcc.manifest and
> ./testing/config/tooltool-manifests/androidarm_6_0/mach-emulator.manifest
> stick out. (I'm not sure if those are actually used anywhere).

Ah, thanks. I missed those. I'll update them as well and rebase around conflicting changes.
Comment hidden (mozreview-request)

Comment 11

a year ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/faf87c0a0f98
Indent tooltool manifests. r=mshal

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/faf87c0a0f98
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

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