Closed Bug 1547325 Opened 5 years ago Closed 5 years ago

README for Thunderbird (comm repository)

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 71.0

People

(Reporter: ryan, Assigned: ryan, Mentored)

Details

Attachments

(1 file, 7 obsolete files)

new-readme-for-thunderbird.patch
11.77 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review

We should have a README for the comm repository (aka Thunderbird) to help provide some guidance to those who pull down the source code.

The README should accomplish the following:

  • Explain the Mozilla Codebase
  • Teach how to build Thunderbird
  • Teach how to run your build
  • Point to how to contribute
  • Explain how to submit a patch
Assignee: nobody → ryan
Attached patch readme-for-thunderbird.patch (obsolete) — — Splinter Review

Patch for the README, please review.

Attachment #9061044 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9061044 [details] [diff] [review]
readme-for-thunderbird.patch

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

Could you at the same time remove the readme.html

It's a good start. Some comments below

Not sure if we need to mention SeaMonkey somewhere...

::: README.md
@@ +1,2 @@
> +# Thunderbird
> +Thunderbird an powerful and customizable open source Email client with lots of users. It is based on the Mozilla platform that Firefox is also built on.

Thunderbird *is* ...
Why Email with capital?
Maybe "It is based on the same platform that Firefox is using."

@@ +10,5 @@
> +In order to be able to build Thunderbird - you will need the mozilla-central repository as well as the comm-central repository (where this README lives). Check out our [Getting Started documentation](https://developer.thunderbird.net/the-basics/getting-started) for instructions on how and where to get the source code.
> +
> +### mozilla-central vs. comm-central
> +
> +Mozilla-central will build Firefox without the comm-central repo present and a few options set \(detailed below). Mozilla-central is the Firefox codebase and comm-central features the additions that turn Firefox into Thunderbird.

The mozilla-central repostitory contains the Firefox codebase and all of the platform code. The comm-central repository is added as a subdirectory "comm/" under mozilla-central. This contains the code for Thunderbird.

@@ +24,5 @@
> +- [macOS Build Prerequisites](https://developer.thunderbird.net/the-basics/building-thunderbird/macos-build-prerequisites)
> +
> +### Build Configuration
> +
> +To build Thunderbird, you need to create a file named `mozconfig` to the root directory of the mozilla-central checkout that contains the option `comm\mail` enabled. You can create a file with this line by doing this in the `source/` directory:

comm/mail

@@ +40,5 @@
> +_Each of these ac\_add\_options entries needs to be on its own line._
> +
> +For more on configuration options, see the page [Configuring build options](https://developer.mozilla.org/en/Configuring_Build_Options). Note that if you use an MOZ\_OBJDIR it cannot be a sibling folder to your source directory. Use an absolute path to be sure!
> +
> +### Build the Lightning Calendar when building Thunderbird  

some trailing space here

@@ +42,5 @@
> +For more on configuration options, see the page [Configuring build options](https://developer.mozilla.org/en/Configuring_Build_Options). Note that if you use an MOZ\_OBJDIR it cannot be a sibling folder to your source directory. Use an absolute path to be sure!
> +
> +### Build the Lightning Calendar when building Thunderbird  
> +
> +Add the following line to your `mozconfig` file:

.mozconfig

@@ +74,5 @@
> +```text
> +./mach run
> +```
> +
> +There are various command line parameters you can add, e.g. to specify a profile.

, such as: -no-remote -P testing --purgecaches

@@ +156,5 @@
> +If you want to ask questions about how to hack on Thunderbird, the IRC channel you want to join is [\#maildev on irc.mozilla.org](irc://irc.mozilla.org/maildev).
> +
> +### Report a Bug and Request Features
> +
> +### [Bugzilla](https://bugzilla.mozilla.org)

https://bugzilla.mozilla.org/enter_bug.cgi?product=Thunderbird

@@ +158,5 @@
> +### Report a Bug and Request Features
> +
> +### [Bugzilla](https://bugzilla.mozilla.org)
> +
> +Thunderbird uses Mozilla's Bugzilla platform to report and track bugs. The site can also be used to generate enhancement bugs, which can be used for feature requests. If you want to become a contributor to Thunderbird, you will need an account on Bugzilla as you will submit patches through this platform.

I think bugzilla is it's own project, so perhaps just say "Thunderbird uses bugzilla to reporting and tracking bugs as well as enhancement requests."
The " as you will submit patches through this platform" part I think we can drop (only half true with phab coming, and it's not a platform but a tool.

@@ +162,5 @@
> +Thunderbird uses Mozilla's Bugzilla platform to report and track bugs. The site can also be used to generate enhancement bugs, which can be used for feature requests. If you want to become a contributor to Thunderbird, you will need an account on Bugzilla as you will submit patches through this platform.
> +
> +### Fixing a Bug and Submitting Patches
> +
> +All the issues, bugs, work in progress patches, or updates related to Thunderbird, are listed on [BugZilla](https://bugzilla.mozilla.org), and are properly organized per **Product**, **Component**, and **Status**.

I'd add the link directly listing recent Thunderbird bugs

@@ +170,5 @@
> +Creating an account is necessary in order to submit patches, leave comments, and interact with any other aspect of BugZilla. If you're currently using an `IRC` username in the `#maildev` channel, we recommend saving your profile name with the current format `Firstname Lastname (:username)` in order to be easily searchable and allow the Thunderbird team to offer better support.
> +
> +#### Find a Bug
> +
> +Use the [Advanced Search](https://bugzilla.mozilla.org/query.cgi?format=advanced) section to find bugs you want to take care of, and be sure that the bug doesn't currently have any user listed as _Assignee_ and the _Status_ is set to `NEW`.

Here too add the direct link for thunderbird, so the search is restricted to Thunderbird, Mailnews, Calendar and Chat

@@ +172,5 @@
> +#### Find a Bug
> +
> +Use the [Advanced Search](https://bugzilla.mozilla.org/query.cgi?format=advanced) section to find bugs you want to take care of, and be sure that the bug doesn't currently have any user listed as _Assignee_ and the _Status_ is set to `NEW`.
> +
> +#### Search for code references

some of the headers are capitalized, some not, please make it consistant

@@ +175,5 @@
> +
> +#### Search for code references
> +
> +Making sense of the **Thunderbird** source code, and knowing where to look, will take some time. The code base is pretty big and if you never worked with `XBL` or `Custom Elements` it can be overwhelming at first. We recommend using our code search engine, [SearchFox](https://searchfox.org/comm-central/source/), to inspect the source code and find snippets and references to help you out while investigating a bug.
> +

Searchfox (lower case F)

@@ +190,5 @@
> +0. Run `hg pull` to download the latest changes from the remote repository.
> +1. Run `hg update tip` to update your current working directory to the most recent changeset that you just downloaded.
> +1. Run the command `hg bookmark some-bookmark-name` to create a bookmark that points to the current changeset. Usually the name of the bookmark is related to the bug or feature you are working on.
> +2. Run `hg bookmarks` to list all of your bookmarks and see which one is currently active.  The one you just created should now be active.
> +2. You can run `hg log` to see which bookmarks point to which changesets.  Or you may prefer using a GUI tool like [TortoiseHG](https://tortoisehg.bitbucket.io/) that provides a more graphical overview.

Not sure I'd want to promote TortoiseHG. I doubt contributors really should be using it.

@@ +205,5 @@
> +Run `hg export --rev feature-A > my-feature-a-file.patch` to create a patch file.  The `--rev feature-A` part indicates which revision (changeset) you want, in this case the changeset that the bookmark `feature-A` points to.
> +
> +#### Upload a Patch
> +
> +Open your `patch-name.patch` file in your code editor and be sure it includes all your code changes, and your name and commit message at the top.

maybe a sample?

@@ +211,5 @@
> +If everything looks good, you can access the selected bug in [BugZilla](https://bugzilla.mozilla.org) and click on the **Attach File** link located above the first comment.
> +
> +#### Ask for a Review
> +
> +When uploading a patch to BugZilla, you can request a review from the user who opened the bug or another developer. Simply select the `?` in the dropdown selector in the _review_ option of the **Flags** section. An input field will appear which will allow you to type the name or username of the user you want to review your patch.
\ No newline at end of file

Bugzilla is also with lower-case z
Attachment #9061044 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached patch readme-for-thunderbird-rev1.patch (obsolete) — — Splinter Review

Here is the patch with your previous review feedback included and the readme.html in the mail directory removed.

Attachment #9061044 - Attachment is obsolete: true
Attachment #9063661 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9063661 [details] [diff] [review]
readme-for-thunderbird-rev1.patch

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

::: README.md
@@ -1,2 @@
>  # Thunderbird
> -Thunderbird an powerful and customizable open source Email client with lots of users. It is based on the Mozilla platform that Firefox is also built on.

Why remove that sentence?
Attached patch readme-for-thunderbird-rev2.patch (obsolete) — — Splinter Review

Didn't actually mean to remove that sentence, just adjust it. New patch takes care of that Jorg.

Attachment #9063661 - Attachment is obsolete: true
Attachment #9063661 - Flags: review?(mkmelin+mozilla)
Attachment #9069788 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9069788 [details] [diff] [review]
readme-for-thunderbird-rev2.patch

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

Should be ok. But this looks like just an incremental patch. You should include the complete patch for the README.md

::: README.md
@@ +25,4 @@
>  
>  ### Build Configuration
>  
> +To build Thunderbird, you need to create a file named `mozconfig` to the root directory of the mozilla-central checkout that contains the option `comm/mail` enabled. You can create a file with this line by doing this in the `source/` directory:

there's no "source" directory.  It's just the root source directory you mean I think
Attachment #9069788 - Flags: review?(mkmelin+mozilla)
Attached patch readme-for-thunderbird-rev3.patch (obsolete) — — Splinter Review

Magnus, this addresses the changes you requested and both patches have been merged into one.

I think I did all that correctly, so please review at your earliest convenience.

Attachment #9069788 - Attachment is obsolete: true
Attachment #9073969 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9073969 [details] [diff] [review]
readme-for-thunderbird-rev3.patch

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

::: README.md
@@ +24,5 @@
> +- [macOS Build Prerequisites](https://developer.thunderbird.net/the-basics/building-thunderbird/macos-build-prerequisites)
> +
> +### Build Configuration
> +
> +To build Thunderbird, you need to create a file named `mozconfig` to the root directory of the mozilla-central checkout that contains the option `comm/mail` enabled. You can create a file with this line by doing this in the root source directory:

We're talking both about .mozconfig and mozconfig now. 
Maybe they changed it to allow just mozconfig too due to Windows... but maybe it would be nice to mention that you can use .mozconfig, and then be consistent abotut which file name used

@@ +52,5 @@
> +### Building
> +
> +**Before you start**, make sure that the version you checked out is not busted. For `hg` tip, you should see green Bs on [https://treeherder.mozilla.org/\#/jobs?repo=comm-central](https://treeherder.mozilla.org/#/jobs?repo=comm-central)
> +
> +To start the build, cd into the `source` directory, and run:

plain source, there is no directory named source

@@ +98,5 @@
> +
> +or to do it via one command:
> +
> +```text
> +hg pull -u; (cd comm; hg pull -u)

I think you want

hg pull -u && cd comm && hg pull -u

Otherwise, on failures you may get into problems (or at least confusions).
Attachment #9073969 - Flags: review?(mkmelin+mozilla) → review+

Any intentions to land this?

Flags: needinfo?(ryan)
Flags: needinfo?(mkmelin+mozilla)

Yeah, Ryan said he's working on fixing up the patch.

Flags: needinfo?(ryan)
Flags: needinfo?(mkmelin+mozilla)
Attached patch new-readme-for-thunderbird.patch (obsolete) — — Splinter Review

Here is the newest patch with the changes requested. One commit, as requested.

Attachment #9073969 - Attachment is obsolete: true
Attachment #9085195 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9085195 [details] [diff] [review]
new-readme-for-thunderbird.patch

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

Looks good, let's land!
Attachment #9085195 - Flags: review?(mkmelin+mozilla) → review+
Type: enhancement → task

Some more comments:

As noted in the mailing list thread - MDN is going to remove our stuff. We need to update DTN regularly with the relevant information.

(In reply to Jorg K (GMT+2) from comment #14)

Some more comments:

  • Who's going to keep this up-to-date?
    You mean DTN? I've tried to get developer's involved to review the content and ensure it is up-to-date.

I will discuss this with Magnus today and see if we can get this task on a staff member's plate to check on these things regularly.

I used what Magnus recommended putting there. But this could be something different. What do you think is most useful?

  • Why are you only mentioning hg bookmarks. Some people use hg queues, so IMHO it should at least be mentioned.
    I've had some folks point out that hg queues is deprecated and probably not a good thing to encourage.

We do have a full section dedicated to Queues on DTN -> https://developer.thunderbird.net/contributing/fixing-a-bug/using-mercurial-queues

I'm open to feedback. So far I've had a hard time getting any attention to this README. Happy to get feedback.

I'll get back to this.

Flags: needinfo?(jorgk)

"Who's going to keep this up-to-date?" was referring to the readme itself. I guess it doesn't have too many details which will go out-of-date soon, but it's another location with build information which potentially will need updating.

I'd remove/replace the link.

I know that hg queues are supposedly deprecated, but everyone is using them. So would at least mention them.

Giving users too many options up front is often more cause of confusion rather than being helpful.

(In reply to Jorg K (GMT+2) from comment #17)

"Who's going to keep this up-to-date?" was referring to the readme itself. I guess it doesn't have too many details which will go out-of-date soon, but it's another location with build information which potentially will need updating.

I will keep it up-to-date with a recurring calendar reminder to check on it and update it.

I'd remove/replace the link.

What would you replace it with? Happy to do it, just want to know what a good alternative would be.

I know that hg queues are supposedly deprecated, but everyone is using them. So would at least mention them.

The best I can do here is mention "You can also use hg queues to manage your patches to Thunderbird, you can learn more about them here."

Flags: needinfo?(jorgk)

What do you think Jorg?

Flags: needinfo?(jorgk)

I'd change the paragraph to:

Mercurial is pretty flexible in terms of allowing writing your own code and keeping it separate from the main code base. You can use using Mercurial Bookmarks or Mercurial Queues or for managing your work. We have guides called for bookmarks and queues on our developer website.

I've discussed the issue with John, and personally I don't think bookmarks are up to doing the job. So I'd not recommend anything. Mention bookmarks first if you want, since B comes before Q ;-)

As for the query. I'd but this:

Use the Advanced Search section to find bugs you want to take care of, and be sure that the bug doesn't currently have any user listed as Assignee and the Status is set to NEW. You can see a list of "easy" bugs for beginners via this query. However, we assume you came here to fix your "pet hate" bug, so you already have a bug to work with.

BTW, the query is: https://mzl.la/2HbDSyX

r+ from me with those changes, haha.

Flags: needinfo?(jorgk)
Attached patch new-readme-for-thunderbird.patch (obsolete) — — Splinter Review

Here is the new patch with the changes requested. Looking forward to a r+! :)

Attachment #9085195 - Attachment is obsolete: true
Attachment #9087157 - Flags: review?(jorgk)
Comment on attachment 9087157 [details] [diff] [review]
new-readme-for-thunderbird.patch

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

I can fix the link. Whoever comes first.

::: README.md
@@ +170,5 @@
> +Creating an account is necessary in order to submit patches, leave comments, and interact with any other aspect of Bugzilla. If you're currently using an `IRC` username in the `#maildev` channel, we recommend saving your profile name with the current format `Firstname Lastname (:username)` in order to be easily searchable and allow the Thunderbird team to offer better support.
> +
> +#### Find a Bug
> +
> +Use the [Advanced Search](https://bugzilla.mozilla.org/query.cgi?format=advanced) section to find bugs you want to take care of, and be sure that the bug doesn't currently have any user listed as _Assignee_ and the _Status_ is set to `NEW`. You can see a list of "easy" bugs for beginners [via this query](https://mzl.la/2HbDSyX). However, we assume you came here to fix your "pet hate" bug, so you already likely have a bug to work with.

Sorry, I didn't suggest to use that short query. I'm not sure how permanent it is. Please use the longer one I provided in the same e-mail. r+ with that.
Attachment #9087157 - Flags: review?(jorgk) → review+

I've come to fix the link and land this, but it's still not right :/

Here's the bookmark example:

  1. Run hg pull to download the latest changes from the remote repository.
  2. Run hg update tip to update your current working directory to the most recent changeset that you just downloaded.
  3. Run the command hg bookmark some-bookmark-name to create a bookmark that points to the current changeset. Usually the name of the bookmark is related to the bug or feature you are working on.
  4. Run hg bookmarks to list all of your bookmarks and see which one is currently active. The one you just created should now be active.
  5. You can run hg log to see which bookmarks point to which changesets.
  6. Make changes to the code until you are ready to commit them.
  7. Run hg status and hg diff to check what files and changes you are about to commit. If you need to add a new file use hg add some-filename before committing.
  8. Commit your changes with hg commit -m "Bug ##### - fixing something amazing". The -m is short for --message and lets you provide a commit message for your changeset. "Bug ##### - fixing something amazing" is the format we recommend for the commit message, specifying the number of the Bug you're working on and a small description stating what you fixed. Use hg log to see examples of commit messages.
  9. Run hg log and note how the bookmark has moved to the changeset that you just committed. The active bookmark will automatically move to the most recent changeset as you commit your changes. That way you can make a series of commits and the bookmark will always point to the most recent one.
  10. You can use hg commit --amend to change your previous commit rather than making a new commit.

You're cheating since you're re-using numbers. Let me give you the much shorter hg queues steps:
0. Run hg pull to download the latest changes from the remote repository.

  1. Run hg update tip to update your current working directory to the most recent changeset that you just downloaded.
  2. Make changes to the code until you think you covered everything.
  3. Create a patch for upload with hg qnew -m "Bug ##### - fixing something amazing". The -m is short for --message and lets you provide a commit message for your changeset. "Bug ##### - fixing something amazing" is the format we recommend for the commit message, specifying the number of the Bug you're working on and a small description stating what you fixed.

The last command will place a patch into .hg/patches in your repository.

You can now scrap the #### Create a New Patch paragraph completely.

So four steps instead of ten seems much simpler, no? Both instructions are missing the information how to get rid of the local changes. You won't need them since the sheriff commits them to the repository and you will pull them from there.

Attachment #9087157 - Flags: review+

I have not gotten rid of my habit of using merqurial queue (this was the method of choice back when I started to create patches).
However, it is quite true that the use of HG MQ is highly discouraged now and there are bugs with |mach bootstrap| that interfere with the proper setting of .hgrc so that MQ works correctly.
So at least, we should mention for the benefit of newbies that "support for HG MQ in various mozilla tools is being deprecated, and you are better off to learn how to use bookmarks".

Attached patch new-readme-for-thunderbird.patch (obsolete) — — Splinter Review

I've thought a lot about this and I am not going to change it to detail mercurial queues as there are a lot of warnings out there in documentation that we shouldn't be using it as it is deprecated. So I have noted that "While some find mercurial queues easier to work with, support for them is being deprecated in various Mozilla tools, so the example below uses bookmarks."

I have also fixed the numbering of the bookmarks instructions.

Let me know what you think, if there is nothing else I would like to see this merged.

Attachment #9087157 - Attachment is obsolete: true
Attachment #9091103 - Flags: review?(jorgk)
Comment on attachment 9091103 [details] [diff] [review]
new-readme-for-thunderbird.patch

> Let me know what you think, if there is nothing else I would like to see this merged.

I prefer the bookmark section to be removed. We have the link to the documentation, so let's refer people there. There's no need to have the same information in two locations. As per the maildev mailing list thread, the bookmark page needs work, so no point copying an incomplete page into the readme.

Also, please check the "space after full stop" style. The typewriter days are over, so we no longer use two spaces after a full stop. Search for ".  " (dot space space) in the document.

Sorry about anther iteration here.
Attachment #9091103 - Flags: review?(jorgk)

Hoping this is the final pass. I couldn't find any double spaces that you pointed out at the end of sentences. I did a search in three editors finders and then went line-by-line and checked. Can you point me at where you see one?

I completely removed the hg bookmarks example and just pointed them at documentation. Kept the rest in the READE about what to do after exporting the patch. Let me know what you think.

Attachment #9091103 - Attachment is obsolete: true
Attachment #9093146 - Flags: review?(jorgk)
Comment on attachment 9093146 [details] [diff] [review]
new-readme-for-thunderbird.patch

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

OK, to avoid further iterations, I'll fix the issues below and land it.

::: README.md
@@ +160,5 @@
> +### [Bugzilla](https://bugzilla.mozilla.org/enter_bug.cgi?product=Thunderbird)
> +
> +Thunderbird uses bugzilla for reporting and tracking bugs as well as enhancement requests. If you want to become a contributor to Thunderbird, you will need an account on Bugzilla.
> +
> +## Fixing a Bug and Submitting Patches

That was a level ### header, and I don't know why you've changed it to level ## since #### are following.

@@ +184,5 @@
> +Once you finished taking care of your favorite bug and using Mercurial to commit and export your patch, you can upload it to Bugzilla for review.
> +
> +#### Upload a Patch
> +
> +Open your `patch-name.patch` file in your code editor and be sure it includes all your code changes, and your name and commit message at the top. You can see an example of a patch for this [README here](https://bug1547325.bmoattachments.org/attachment.cgi?id=9061044).

I think we can remove the `patch-name.patch`  bit here. Maybe that was referenced before in the sections your removed. Also, the ID of the attachment has changed.
Attachment #9093146 - Flags: review?(jorgk) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/97972953d726
Created README for Thunderbird with build instructions, how to get involved in the community, and a basic guide for contributing. r=mkmelin,jorgk

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: