1317630 - Allowable calendar duplicates need "distribution" prepended to the path on Aurora

Status: RESOLVED WONTFIX

(Calendar :: Build Config, defect, P3)

Description

[Jorg K (CEST = GMT+2)]

After uplift on 14th Nov. 2016 Aurora was busted at first since at packaging time about 50 (depending on the platform) duplicate files were detected.

All these were all of the form
distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/...

All these files were already listed in mail/installer/allowed-dupes.mn but without the "distribution" at the front.

I fixed the bustage by prepending "distribution" as can be seen here:
https://hg.mozilla.org/releases/comm-aurora/rev/2685928dd367e321c6140fd1bb4cf94c5ac12673

We need a permanent solution for further uplifts, not just to avoid the disappointment of the person doing the uplift, presently Patrick, when seeing an "awful" (Patrick's words) result ;-)

Perhaps we can put something conditional into allowed-dupes.mn or perhaps we should investigate why there is "distribution" at the front in Aurora.

Comment 1

[Richard Marti (:Paenglab)]

Lightning is packaged in /distribution on all versions other than Daily.

Maybe we need something like https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/installer/find-dupes.py#32 to remove the leading, differing, path.

Comment 2

[Jorg K (CEST = GMT+2)]

Fix for merge day on Jan 23rd, 2017:
https://hg.mozilla.org/releases/comm-aurora/rev/c52e9a6783a82fe7b55ef280a879d20effa89175

Comment 3

[Frank-Rainer Grahl (:frg)]

I ran into the same problem for SeaMonkey in Bug 1314892. Wasn't fatal because we still have the -w switch in place (which will probably stay because onf an l10n problem with the find code for now). My solution was to just duplicate the lines. IanN suggested a separate calendar-dupes file. I asked fallen but haven't heard back yet.

> Lightning is packaged in /distribution on all versions other than Daily.

Is this historical/needed or could this probably be changed?

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Frank-Rainer Grahl from comment #3)
> I ran into the same problem for SeaMonkey in Bug 1314892. Wasn't fatal
> because we still have the -w switch in place (which will probably stay
> because onf an l10n problem with the find code for now). My solution was to
> just duplicate the lines. IanN suggested a separate calendar-dupes file. I
> asked fallen but haven't heard back yet.
Sorry I haven't replied. I hope I am on needinfo for that bug, that will increase chances of getting a reply. I haven't gotten around to my queue lately. Generally I am fine with a calendar-specific file though.

> 
> > Lightning is packaged in /distribution on all versions other than Daily.
> 
> Is this historical/needed or could this probably be changed?

It is in /extensions for aurora/nightly for easier testing. I could imagine changing that condition so it is only in /extensions for local builds, but unless there is a strong need for this I think it should stay.

Comment 5

[Jorg K (CEST = GMT+2)]

Fix for merge day on March 6/7th, 2017:
https://hg.mozilla.org/releases/comm-aurora/rev/5c9c74ff61313eb7bc3eeaf306adf6e768121ec2

Comment 6

[aleth [:aleth]]

Maybe these additional paths should just be added to c-c as well, to avoid having to remember this after every merge?

Comment 7

[Jorg K (CEST = GMT+2)]

See comment #3 and comment #4. I'd appreciate a solution here, since I always interfere with Patrick's merges (which he doesn't always appreciate).

Comment 8

[Patrick Cloke [:clokep]]

We can script this change, we do similar things in a lot of places to during merges, see https://hg.mozilla.org/users/bugzilla_standard8.plus.com/drivertools/file/tip/comm-merges/merge-central-2-aurora.sh

That seems like a hack, however.

Comment 9

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #7)
> See comment #3 and comment #4. I'd appreciate a solution here, since I
> always interfere with Patrick's merges (which he doesn't always appreciate).

What I was suggesting was to have two entries (one for each path) for each calendar allowed dupe. I doubt anything checks if there are "incorrect" entries in allowed-dupes. It's not pretty, but it's quicker than writing a script.

Comment 10

[Jorg K (CEST = GMT+2)]

Yes, that was suggested in comment #3 (quote): My solution was to just duplicate the lines.
But somehow that wasn't well received.

Comment 11

[Jorg K (CEST = GMT+2)]

Note that since bug 1315309, the list is pre-processed, so you could prepend something like
  @CAL_DIST_DIR@extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/linux/lightning/imip.css
where @CAL_DIST_DIR@ would evaluate to |distribution/| for C-A and above.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Jorg K (GMT+2) from comment #11)
> Note that since bug 1315309, the list is pre-processed, so you could prepend
> something like
>  
> @CAL_DIST_DIR@extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/
> linux/lightning/imip.css
> where @CAL_DIST_DIR@ would evaluate to |distribution/| for C-A and above.

I think this is a good idea. Anyone willing to spin up a patch?

Is this broken again given we just merged?

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen] from comment #12)
> Is this broken again given we just merged?
No, it breaks when merging from C-C to C-A and in the future from C-C to C-B. Since there wasn't a C-C to C-A merge, nothing broke. I'm willing but lacking knowledge :-(

Comment 14

[Jorg K (CEST = GMT+2)]

We've added the necessary duplicates in bug 1358877 now
https://hg.mozilla.org/comm-central/rev/f150b3f8e64eddfd26d95ab1525f2d5c46af7e7a#l3.12
but that doesn't mean that we can't have a more elegant solution.

Comment 15

[Martin Schröder [:mschroeder]]

Attachment: patch v1

Comment 16

[Martin Schröder [:mschroeder]]

Comment on attachment 8876510 [details] [diff] [review]
patch v1

Just tested locally. I also aligned the Lightning sections in allowed-dupes.mn for Thunderbird and Seamonkey.

Comment 17

[Frank-Rainer Grahl (:frg)]

Martin did you try it with Nightly? Just got an undefined variable here with a SeaMonkey build. Doing a new one now where I did a configure first.

Comment 18

[Frank-Rainer Grahl (:frg)]

No go unfortunately.

+++ snip +++

PLEMENTATION=1 -DU_USING_ICU_NAMESPACE=0 -DVPX_X86_ASM=1 -DWIN32=1 -DWIN32_LEAN_AND_MEAN=1 -DWINVER=0x601 -DXP_WIN=1 -DXP_WIN32=1 -D
X_DISPLAY_MISSING=1 -D_AMD64_=1 -D_CRT_NONSTDC_NO_WARNINGS=1 -D_CRT_SECURE_NO_WARNINGS=1 -D_USE_MATH_DEFINES=1 -D_WIN32_IE=0x0800 -D
_WIN32_WINNT=0x601 -D_WINDOWS=1 -DAB_CD=en-US -w -f d:/seamonkey/comm-central/suite/installer/allowed-dupes.mn -f d:/seamonkey/comm-
central/mozilla/browser/installer/allowed-dupes.mn  ../../dist/seamonkey
Traceback (most recent call last):
  File "d:/seamonkey/comm-central/mozilla/toolkit/mozapps/installer/find-dupes.py", line 135, in <module>
    main()
  File "d:/seamonkey/comm-central/mozilla/toolkit/mozapps/installer/find-dupes.py", line 128, in main
    pp.do_include(filename)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 776, in do_include
    self.handleLine(l)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 554, in handleLine
    self.write(aLine)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 435, in write
    filteredLine = self.applyFilters(aLine)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 410, in applyFilters
    aLine = f[1](aLine)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 732, in filter_substitution
    return self.varsubst.sub(repl, aLine)
  File "d:\seamonkey\comm-central\mozilla\python\mozbuild\mozbuild\preprocessor.py", line 730, in repl
    raise Preprocessor.Error(self, 'UNDEFINED_VAR', varname)
mozbuild.preprocessor.Error: ('d:\\seamonkey\\comm-central\\suite\\installer\\allowed-dupes.mn', 283, 'UNDEFINED_VAR', 'LTN_DIST_DIR
')
d:/seamonkey/comm-central/mozilla/toolkit/mozapps/installer/packager.mk:39: recipe for target 'stage-package' failed
mozmake[3]: *** [stage-package] Error 1
mozmake[3]: Leaving directory 'd:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/suite/installer'
Makefile:41: recipe for target 'installer' failed
mozmake[2]: *** [installer] Error 2
mozmake[2]: Leaving directory 'd:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/suite/installer/windows'
Makefile:238: recipe for target 'installer' failed
mozmake[1]: *** [installer] Error 2
mozmake[1]: Leaving directory 'd:/seabuild/release/comm-central-15/obj-x86_64-pc-mingw32/suite/installer'

Comment 19

[Frank-Rainer Grahl (:frg)]

Ah found it. Failed because I didn't compile Lightning in my test VM. ac_add_options --enable-calendar was not set.

Not sure but would probably be best to put the Lightning dupes in a separate file in calendar and just include it in the TB and SeaMonkey installer makefile if calendar building is enabled.

Comment 20

[Martin Schröder [:mschroeder]]

(In reply to Frank-Rainer Grahl (:frg) from comment #19)

Thanks for the analysis, didn't think about Lightning being optional.

Comment 21

[Frank-Rainer Grahl (:frg)]

I always just set ENABLE_CALENDAR=1 in confvars.sh and subsequently forget to change the copied mozconfigs when I compile a clean tree in my other test VM. Was actually a good thing this time :)

Comment 22

[Edmund Wong (:ewong)]

Comment on attachment 8876510 [details] [diff] [review]
patch v1

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

lgtm.

Comment 23

[Martin Schröder [:mschroeder]]

Attachment: Patch v2

I updated the patch with the suggestion from :frg. Unfortunately I havent't had a chance to build it because of the permanent failing builds.

Comment 24

[Jorg K (CEST = GMT+2)]

Checkin, or do you want to do a try build (see comment #23)?

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yeah, a try build would probably be a good idea, this would at least allow us to test without distribution/. Bonus points for a try build that pretends not to be a nightly. Jörg, can you start one (or both) of these for us?

Comment 26

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dfd0c7bb79ac95e1f69d953d031c7805af80f739

Not a success:
mozbuild.preprocessor.Error: ('', 0, 'FILE_NOT_FOUND', 'c:\\builds\\moz2_slave\\tb-try-c-cen-w32-0000000000000\\build\\objdir-tb\\mail\\installer\\../../calendar/lightning/allowed-dupes.mn')
mozmake.exe[3]: *** [stage-package] Error 1

I'm not sure why the patch does:
MOZ_PKG_DUPEFLAGS += -f $(DEPTH)/calendar/lightning/allowed-dupes.mn
when others do other things:
https://dxr.mozilla.org/comm-central/search?q=allowed-dupes.mn&redirect=false

Please play with this yourself until it's ready. Re. comment #23: "permanent failing builds". For a while the tree hasn't been closed and you can practically always build when C-C builds, which is practically all the time unless I'm just in the process of fixing bustage.

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks Jörg! I suspect we need to use the right combination of topsrcdir variables (keeping in mind that sometimes topsrcdir is m-c). Martin, can you take a look?

Comment 28

[Martin Schröder [:mschroeder]]

Attachment: Patch v3

I updated the patch with fixes for the correct srcdir path and using the LTN_DIST_DIR variable in mail/suite. Also I am carrying forward the review by Philipp.

The successful try build is: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=85933f8fb0f4b80129507594d8c21cbe40037cc6

The unsuccessful try builds were:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=79bbcdbe9b7271f42cf1f123ab44d9e040134fc3
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=60d403eb38aeed1f3c4eabd6bb5c80f3b61efc9f

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks for tweaking this until it worked :-) Which information would you like me to provide? I don't see a question. However, I have a question:
+DEFINES += -DMOZ_CALENDAR=1 \
+           -DLTN_DIST_DIR='$(LTN_DIST_DIR)' \
+           $(NULL)
What does the $(NULL) do at the end? I can see it used in many occasions.

Comment 30

[Martin Schröder [:mschroeder]]

(In reply to Jorg K (GMT+2) from comment #29)
> Thanks for tweaking this until it worked :-) Which information would you
> like me to provide? I don't see a question. However, I have a question:
> +DEFINES += -DMOZ_CALENDAR=1 \
> +           -DLTN_DIST_DIR='$(LTN_DIST_DIR)' \
> +           $(NULL)
> What does the $(NULL) do at the end? I can see it used in many occasions.

Sorry, I just wanted you to see that I checked the updated patch thoroughly before asking for check-in.

To answer your question: "A backslash as the last character on a line allows the variable definition to be continued on the next line. The extra whitespace is compressed. The terminating $(NULL) is a method for consistency; it allows you to add and remove lines without worrying about whether the last line has an ending backslash or not." (from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/How_Mozilla_s_build_system_works)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/71094089bd57
Allowable calendar duplicates need distribution/ prepended to the path on non-nightly builds. r=philipp

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 8904064 [details] [diff] [review]
Patch v3

Let's try this in a calendar beta. Better to have it break now than at the next merge day ;-)

Comment 33

[Jorg K (CEST = GMT+2)]

Beta (TB 56, Calendar 5.8):
https://hg.mozilla.org/releases/comm-beta/rev/a4e53b458b64d08c47a5b37a2d5fddf8f454027c

Comment 34

[Jorg K (CEST = GMT+2)]

Hmm, not such a success on beta:

Mac:
WARNING: Found 24 duplicated files taking 80415 bytes (uncompressed)
ERROR: The following duplicated files are not allowed:
DailyDebug.app/Contents/Resources/distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/linux/lightning/imip.css
DailyDebug.app/Contents/Resources/distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/windows/lightning/imip.css

Windows:
WARNING: Found 19 duplicated files taking 63099 bytes (uncompressed)
ERROR: The following duplicated files are not allowed:
distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/linux/lightning/imip.css
distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/windows/lightning/imip.css

Linux:
WARNING: Found 19 duplicated files taking 67503 bytes (uncompressed)
ERROR: The following duplicated files are not allowed:
distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/linux/lightning/imip.css
distribution/extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/skin/windows/lightning/imip.css

I'll have to take it out for now. Backouts:
https://hg.mozilla.org/comm-central/rev/e06fa0989673d8f0fe9ca4ea8001bde5e0b9bd98
https://hg.mozilla.org/releases/comm-beta/rev/09be3d607958b70c9b617bc304c41ea2f7badf7f

Comment 35

[Worcester12345]

Any update on this?

Comment 36

[Jorg K (CEST = GMT+2)]

Nope :-(

Comment 37

[Martin Schröder [:mschroeder]]

This bug should be invalid after landing bug 1608610.

Comment 38

[Jorg K (CEST = GMT+2)]

Well, it was valid at the time, but no need to fix it any more.