875059 - Flatten comm-central directories

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Joshua Cranmer [:jcranmer]]

The changes I think ought to be made:
mailnews/base/search -> mailnews/search
mailnews/import/*/src -> mailnews/import/* [every importer should just be one level deep]
Flatten mailnews/mapi/mapihook/*
Flatten every directory in mailnews/extensions/*
  e.g., we would have
  bayesian-spam-filter/nsIncompleteGamma.h
  bayesian-spam-filter/nsBayesianFilter.cpp
  bayesian-spam-filter/test/xpcshell.ini [drop the unit/ from the name]
  [I may make an exception for mailnews/extensions/smime/content]
mail/components/search/public -> mail/components/search
mail/components/shell/public -> mail/components/shell
mail/components/migration/* -> mail/components/migration
db/mork/* -> db/mork or maybe even mailnews/db/mork

Comment 1

[:aceman]

Does this propose to drop the */content */src */public separate directories?

Comment 2

[rsx11m]

It sure would be good to keep things reasonably consistent. For example, why would you consolidate mail/components/migration/{content,public,src} but not do so also for mailnews/compose/{content,public,src} and directories with similar structure? Because the directories in question are one level deeper?

Is this related to or needed for building comm-central within mozilla-central?

Comment 3

[Joshua Cranmer [:jcranmer]]

(In reply to rsx11m from comment #2)
> It sure would be good to keep things reasonably consistent. For example, why
> would you consolidate mail/components/migration/{content,public,src} but not
> do so also for mailnews/compose/{content,public,src} and directories with
> similar structure? Because the directories in question are one level deeper?

The main reason has to do with size. The subdirectories of mailnews/ have a large number of IDL files and source files (particularly mailnews/base and mailnews/imap); mail/components/migration, on the other hand, has effectively a single IDL file and a few CIDs.

> Is this related to or needed for building comm-central within
> mozilla-central?

Nope. Just some personal issues I have with the layout.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Relocate the mork directories

Of the directory reorganizations, this is probably the most useful. It eliminates a top-level directory that collides with mozilla/. While I was moving the code, I also eliminated the pretense that we can build without it and collapsed the directory structure.

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Relocate mailnews/base/search to mailnews/search

The directory location of base/search never made much sense to me. :-)

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Remove unnecessary directories from import

No sense having directories who only have one child...

Comment 7

[:aceman]

I can kinda understand the import movement, e.g. oexpress and winlivemail didn't have a src already.
The mork move could be useful if mork is no longer a standalone component and we want to "adopt" it if nothing outside c-c is using it any longer. The search move seems arbitrary, but yes, if there is /mailnews/base/public/* and then a deeper one /mailnews/base/search/public/* that may be kinda strange. But then you would have to move prefs too to follow this logic.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8708208 [details] [diff] [review]
Part 1: Relocate the mork directories

I looked at this, but I could not get the xpcshell tests to work. Try server seems to be failing unrelated to this problem, and local builds failed in attempts to verify the database file.

So I don't understand what is wrong, but we need to confirm that xpcshell tests will work after this. Could you do a try tun, or is there something that I need to do different in my local builds? I even tried a full clobber.

This is sort of a feedback+, ask for review again if you can do a successful try server run.

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Relocate the mork directories

Turns out that I forgot to include the file that contains the mork factory factory (can you say overengineered?). Oops.

Comment 10

[Joshua Cranmer [:jcranmer]]

Try build here by the way: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=117e72811693

Comment 11

[Kent James (:rkent)]

Comment on attachment 8710803 [details] [diff] [review]
Part 1: Relocate the mork directories

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

Yes this looks fine, and tests work now.

Comment 12

[Kent James (:rkent)]

Comment on attachment 8708209 [details] [diff] [review]
Part 2: Relocate mailnews/base/search to mailnews/search

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

In general, I am not enthusiastic about moving code around like this, but I would normally yield to you and let you do what you want.

But I cannot in this case, because previously the tests that were associated with search would still be located under mailnews/base/test after this move. So if you want to move mailnews/base/search, you need to locate the tests associated with it, and move them as well.

Comment 13

[Kent James (:rkent)]

Comment on attachment 8708210 [details] [diff] [review]
Part 3: Remove unnecessary directories from import

I have mixed feels about this. Were we to add tests to, say, vcard, the logical place to move that would be to mailnews/import/vcard/test since they would be specific to vcard, as these imports are very different from each other. If we add that, then we have source at mailnews/import/vcard but the tests at mailnews/import/vcard/test  That would be fine if it was the convention, but it is not in the rest of the code.

I suppose that you could argue that tests could be at mailnews/import/test, but that mixes all of these tests together for things that are quite distinct from each other.

So overall, I do not see the point of this move. But I'm going to yield to you and not prevent you from doing this if this is what you want. You can choose whether you want to land this or not.

Comment 14

[Kent James (:rkent)]

Comment on attachment 8708210 [details] [diff] [review]
Part 3: Remove unnecessary directories from import

One more comment. I see that there is an existing test test_vcard_import.js under mailnews/import/test. So already the mixing of tests is occurring, and it is really hard to tell how much testing is being done to this specific part of the code. If vcard is going to be its own subdirectory, then I believe it should have its tests moved there. But I won't require this, as after this patch at least the tests will be in a sister directory, and not in a nephew as would have been the case in the search move.

Where are you going? Is this part of a proposal to eliminate all of the /src directories in mailnews?

Comment 15

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #12)
> But I cannot in this case, because previously the tests that were associated
> with search would still be located under mailnews/base/test after this move.
> So if you want to move mailnews/base/search, you need to locate the tests
> associated with it, and move them as well.

Fair enough. I did think about moving them, but I forgot that I hadn't yet done it when I uploaded the patch.

(In reply to Kent James (:rkent) from comment #14)
> Where are you going? Is this part of a proposal to eliminate all of the /src
> directories in mailnews?

What I want is uniformity in the codebase. Ideally, it would be mailnews/*/{public,src,test,content}, although the mailnews/db and mailnews/extensions are sort of permanently screwed up (and MAPI is screwy because of one-library-per-directory rules). I know mozilla-central generally eliminated the public/src distinction, but I think the benefits of that approach aren't much worth it until/unless we start drastically JS-ifying code, which brings its own ideas of how to organize code.

Import is the directory that can go either way. From a theoretical standpoint, it does seem clean to have each importer be its own self-contained directory. In practice, we're not likely to add any more importers (mbox is the only one I can think of, and we're talking about deleting Eudora), and having all the importers share a directory might have convinced people to share code, avoiding senseless duplication like EudoraImageElement/OutlookImageElement.

(In reply to Kent James (:rkent) from comment #14)
> One more comment. I see that there is an existing test test_vcard_import.js
> under mailnews/import/test. So already the mixing of tests is occurring, and
> it is really hard to tell how much testing is being done to this specific
> part of the code. If vcard is going to be its own subdirectory, then I
> believe it should have its tests moved there. But I won't require this, as
> after this patch at least the tests will be in a sister directory, and not
> in a nephew as would have been the case in the search move.

Import code is highly unlikely to ever be tested very thoroughly. Outlook code in particular appears to be impossible to test except via use of some sort of custom VMs, and that's the code which is actually the most complicated and most desirable to test. So most of the tests are likely to be basic smoke tests where reusing the same basic test framework (the import_helper.js) code is more important than anything else.

Comment 16

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 1: Move mork code from db/mork to mailnews/db/mork. r?mkmelin

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 2: Move mailnews/base/search to mailnews/search. r?mkmelin

Depends on D85670

Comment 18

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 3: Move the CMS code to the rest of the S/MIME code. r?kaie

Since CMS code is only used via the S/MIME code, this is where it ought to live.

Depends on D85671

Comment 19

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 4: Flatten mailnews/extensions/* directories. r?mkmelin

Depends on D85672

Comment 20

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 5: Flatten mailnews/import/* into a single directory. r?mkmelin

Depends on D85673

Comment 21

[Joshua Cranmer [:jcranmer]]

Attachment: Bug 875059, part 6: Move mailnews/base/util to mailnews/base/src. r?mkmelin

Depends on D85674

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dc216c691deb
part 1: Move mork code from db/mork to mailnews/db/mork. r=mkmelin
https://hg.mozilla.org/comm-central/rev/05d8dba40703
part 2: Move mailnews/base/search to mailnews/search. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e12a85678599
part 3: Move the CMS code to the rest of the S/MIME code. r=kaie
https://hg.mozilla.org/comm-central/rev/29a2565ddcb3
part 4: Flatten mailnews/extensions/* directories. r=mkmelin
https://hg.mozilla.org/comm-central/rev/34e323c279b1
part 5: Flatten mailnews/import/* into a single directory. r=mkmelin
https://hg.mozilla.org/comm-central/rev/308f24db1bf3
part 6: Move mailnews/base/util to mailnews/base/src. r=mkmelin

Comment 23

[alta88]

Was this change done without any consideration whatsoever for history? Why were these files copied rather than using hg rename? Why weren't these changesets excluded from hg history, a la whitespace change best practice.

This should be backed out and done properly, assuming it even has legitimate value to begin with. Yet another example of ill considered rubber stamping.

Comment 24

[Joshua Cranmer [:jcranmer]]

(In reply to alta88 from comment #23)

Was this change done without any consideration whatsoever for history? Why were these files copied rather than using hg rename?

hg rename is actually implemented as a copy-and-delete under the hood. I ran hg mv for all of these files.

Well, not all of them. One set of files was an hg cp followed by hg rm. If you can't figure out which set of files that was from the changesets, then that should dispel your accusations. (Spoiler alert: I've already checked, and there is no observable difference for this set of files).

Comment 25

[:aceman]

Yeah, e.g. the mork patch uses 'rename' commands, but the history still seems to be lost. hg log mdb.h shows jcranmer as the initial author.

Comment 26

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #25)

Yeah, e.g. the mork patch uses 'rename' commands, but the history still seems to be lost. hg log mdb.h shows jcranmer as the initial author.

Use hg log --follow instead; for some reason, mercurial does not follow moves by default.

Comment 27

[:aceman]

Ok, that seems to work, thanks.

Comment 28

[Jorg K (CEST = GMT+2)]

I think I was a really BAD idea to move stuff around and the beginning of the ESR cycle. There will be uplift problems with no end now(*). Those global changes should be done at the end of an ESR cycle.

You should reconsider: status-thunderbird_esr78: --- → wontfix. Won't hurt to do the same shuffling on the ESR branch like we did for linting and clang-format on other occasions.

(*) Starting here in bug 1653690.

Comment 29

[ISHIKAWA, Chiaki]

I am sorry to say this, it would have been very great if this change had been announced well in advance.
Maybe I missed reading such a post.　But that is also my point.
Change of this magnitude ought to be announced maybe weekly for about 3-4 weeks in advance.

I recall that source formatting done by Jorg was announced way before and it was notified a few times so that everybody was aware of it coming.

Just my idea. YMMV.

Comment 30

[Magnus Melin [:mkmelin]]

Yeah maybe we should take it for esr a bit later.

(In reply to ISHIKAWA, Chiaki from comment #29)

Change of this magnitude ought to be announced maybe weekly for about 3-4 weeks in advance.

How would that have changed anything? Changes like these require adapting patches at the time they happen anyway, not before.

Comment 31

[ISHIKAWA, Chiaki]

In reply to Magnus Melin [:mkmelin] from comment #30)

Yeah maybe we should take it for esr a bit later.

(In reply to ISHIKAWA, Chiaki from comment #29)

Change of this magnitude ought to be announced maybe weekly for about 3-4 weeks in advance.

How would that have changed anything? Changes like these require adapting patches at the time they happen anyway, not before.

In the case of Jorg's reformatting change, I was ready to tackle the major surgery because I had noticed his posts.

I had created the duplicate of the local source tree as well as the local patches and even figure out how to do the copying, reformatting, etc. in advance. (Algorithmically, I found it better to create the final changed tree as a whole first and work backward. Well, I forget the details now, but it worked. Also during verification, I found a couple of copy&paste mistakes done before and fixing these copy&paste errors eliminated a strange mozmill test error I noticed with my local patches. Great.)

The readiness and awareness counted. I was confident of what I was doing.

This time, I was not prepared and so after updating the C-C tree and while I tried to apply the local patches, I trashed the first few patches , thinking, OK, this patch and that patch were already applied (which was the case) in the new C-C tree, only when the subsequent patch failed and when I investigated the file in question was nowhere to be found, I got really confused and I needed to send an inquiry to the mailing list, to which Jorg answered with the pointer to this bugzilla entry.

During the few hours I spent trying to figure out what went wrong, I felt very uneasy since I thought I had trashed my local tree permanently due to some errors, and I eventually gave up to see if the HG commands failed [which is very unlikely these days]. (I checked searchfox and thought it had NOT reflected the patch yet and so it showed the missing file. (?) Now I am not sure. Maybe it showed the file but under the different new directory. But I was so confused then and not noticed it, if so, and I know searchfox database can be a day or two old sometimes.)
At least, I could have avoided this very uneasy few hours of analyzing a non-bug.

That is my perspective.

Anyway, if I had known, I would have been ready with the duplicate source tree and duplicate local patch queue to cope with the changes in advance.
And I would have probably figured out the changes needed and created a shell script to change the paths in the local mqueue patches to automagically update the patches to cope with the newly directory structure.

Yeah, I am sorry to admit that I am still using MQ, which I understand is not recommended these days.
But some shell scripts I created for local development are tightly coupled with MQ and not easy to get away with it yet

Anyway, I would change the patches this coming weekend hopefully.

TIA

Comment 32

[Magnus Melin [:mkmelin]]

Sorry if it caused problems. FWIW, just rebasing your patch queue in hg should have worked automatically - though as you say, mq can't do that. I think to fix up your patch queue you could back up to the revision before this landed, commit all the mq patches, then rebase and (if you want) pull qimport them all away again.

Comment 33

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #32)

Sorry if it caused problems. FWIW, just rebasing your patch queue in hg should have worked automatically - though as you say, mq can't do that. I think to fix up your patch queue you could back up to the revision before this landed, commit all the mq patches, then rebase and (if you want) pull qimport them all away again.

Thank you for your tips. I should get more familiar with HG commands.
(Somehow I got acquainted with git a bit more easily. Don't know why. Maybe a slight difference in mental model? But I can't put my fingers on it.)

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9167516 [details]
Bug 875059, part 1: Move mork code from db/mork to mailnews/db/mork. r?mkmelin

[Approval Request Comment]
User impact if declined: will have rebasing problems for most c++ fixes we take during 78
Risk to taking this patch (and alternatives if risky): Safe, just moving code around.

Comment 35

[Magnus Melin [:mkmelin]]

(For all the changesets)

Comment 36

[Wayne Mery (:wsmwk)]

Comment on attachment 9167521 [details]
Bug 875059, part 5: Flatten mailnews/import/* into a single directory. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 37

[Wayne Mery (:wsmwk)]

Comment on attachment 9167516 [details]
Bug 875059, part 1: Move mork code from db/mork to mailnews/db/mork. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 38

[Wayne Mery (:wsmwk)]

Comment on attachment 9167517 [details]
Bug 875059, part 2: Move mailnews/base/search to mailnews/search. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 39

[Wayne Mery (:wsmwk)]

Comment on attachment 9167518 [details]
Bug 875059, part 3: Move the CMS code to the rest of the S/MIME code. r?kaie

[Triage Comment]
Approved for esr78

Comment 40

[Wayne Mery (:wsmwk)]

Comment on attachment 9167520 [details]
Bug 875059, part 4: Flatten mailnews/extensions/* directories. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 41

[Wayne Mery (:wsmwk)]

Comment on attachment 9167522 [details]
Bug 875059, part 6: Move mailnews/base/util to mailnews/base/src. r?mkmelin

[Triage Comment]
Approved for esr78

Comment 42

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/9e42b6a85af0
https://hg.mozilla.org/releases/comm-esr78/rev/d2c3b003962e
https://hg.mozilla.org/releases/comm-esr78/rev/ea24adc1f34b
https://hg.mozilla.org/releases/comm-esr78/rev/b6db1fd48f59
https://hg.mozilla.org/releases/comm-esr78/rev/ec9fdf84f987
https://hg.mozilla.org/releases/comm-esr78/rev/c9cd765ab75b