Closed Bug 716552 Opened 13 years ago Closed 13 years ago

MPL 2 upgrade: Camino

Categories

(Camino Graveyard :: General, defect)

Product:
Camino Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gerv, Assigned: gerv)

References

Details

(Whiteboard: [camino-2.1.3])

Attachments

(1 file, 1 obsolete file)

Patch v.2
909.28 KB, patch
alqahira
: review+
Details | Diff | Splinter Review 
This bug tracks the MPL 2 upgrade for the project named in the subject line.
The repo is here: http://hg.mozilla.org/camino/

Gerv
Gerv, is there any ETA/timeline on this?  The wiki page doesn't seem to have that info on that, just the logistics for upgrading m-c and c-c at some currently-unspecified time.

Part of the reason I ask is because we're considering creating a branch inside the Camino repo soon for some future development, and ideally we'd like both the branch and "default" to pick up the MPL2.  Obviously that would be easier if the MPL upgrade happened before we cut the branch (but if the upgrade is not going to be soon, we can cut the branch and then do a license upgrade after the fact with some extra work).

But, yay!  We've been waiting a long time for this.
Blocks: 327035, 569180
We can do this as soon as you are ready :-) Let me attach an initial patch for your review.

Gerv
One question: files like 
./resources/localized/English.lproj/global_locale_plugins_properties.strings.in 
have a very strange commenting scheme involving both /* and #. Is that necessary/intentional?

Gerv
When we converted Gecko .properties files into Cocoa .strings files for localization, the quick-and-dirty script wrapped all the .properties-style comment markers+comments with .strings-style markers (s/^#(.*)$/\/\* #\1 \*\//g, or something to that effect), rather than entirely rewriting the comment lines.

I don't recall the double-comments being necessary (I think it was just a function of the quick-and-dirtiness of the script, my weak regexp-fu, and possibly the desire to make as few changes to the files as possible at that point) and the bug doesn't give any indication we discussed it.

Unless Stuart says otherwise, it's OK with me if you want to make all of those *_properties.strings.in files' comments or license blocks end up just with .strings-style comments: /* comment text */
Attached patch Patch v.1 (obsolete) — — Splinter Review 
Try this for size :-)

Gerv
Attachment #587646 - Flags: review?(alqahira)
It'll take me probably a few days to scan through everything and test a couple of things.

One thing I did notice already, though, and can make a firm comment on:

flashblock/content/flashblock.css and flashblock/content/flashblock.xml are not Camino/Mozilla code; they're tri-licensed, but http://flashblock.mozdev.org/ is upstream, so we can't relicense them ourselves, I don't think ;)

(The files are in the same format they exist upstream--both source and "binary" versions are license-less--but we do specifically mention the license in the corresponding README: http://mxr.mozilla.org/camino/source/camino/flashblock/README )
OK; I'll add an exclusion for flashblock/content.

Gerv
A few more things (still working through the patch):

* Please exclude .nib files/directories.  The individual .nib files are part of a larger .nib directory ("package"), and these are not designed to be edited by hand (even though two of the component .nib files are text-based; they should be considered binary output of another application).  Adding a license block to the info/classes.nib files *breaks* the ability of certain versions of Interface Builder to load the nib, and in other versions of Interface Builder that can still open the nib, the license block is not persisted when re-saving the nib after making a change.

* The standard editing applications for the various property list-like files (*.plist, Camino.sdef, Charset.dict) do not persist the added license block when saving changes to those files.  They're not really "code" although they are required parts of the application, but they're not the same class of "software output" as .nib files, either.  FWIW, I've never[*] seen one of these files in the wild with a licence block, perhaps for this very reason.  (* Obviously, I have not looked at every .plist file in every open-source project out there.)  I'd certainly very much prefer it if we did not have to add license blocks that will be stripped on each edit and must be manually reinserted with another editor after an edit, to this collection of files.

* I'm unsure if the following types of files are considered "code" here.  However, adding a license block does not appear to break anything, so if they are code, or if we prefer to err on the side of "adding a license", it's OK to add a license:
** config/mozconfig
** config/app-license.html
** config/*.xcconfig
** resources/application/*.tbl
Attached patch Patch v.2 — — Splinter Review 
OK, try this for size :-)

Gerv
Attachment #587646 - Attachment is obsolete: true
Attachment #587646 - Flags: review?(alqahira)
Attachment #590750 - Flags: review?(alqahira)
Gerv, sorry for the delay in getting back to this; I've been pulled in a bunch of different directions lately :-(  I hope to have time for this patch this weekend…
Smokey: ping?

Gerv
Smokey: ping? :-)

Gerv
This has been blocked on real life and releases, and the need to find time to clear every Camino off of my Mac in order to make sure the Spotlight importer still works with a license header in schema.xml. :-(

Philippe has volunteered ;-) to test the latter for me, so at that point I'll just need to look at the interdiff and then pick up scanning the diff from the point I left off last time.

One other thing while I'm thinking about it now: when we landed the Spotlight importer, Stuart and I discussed the license status for src/spotlight-importer/Importer-main.c (which is why it doesn't have a license right now).  This is an interesting file; Xcode created it entirely, but it's not (apparently) "Apple Sample Code" either:

[1:59pm] sauron: is there a license for src/spotlight-importer/Importer-main.c ?
[1:59pm] smorgan: It's boiler-plate
[2:00pm] smorgan: Putting a license in a file that is 100% Xcode-generated seemed silly
[2:00pm] smorgan: Plus I wouldn't know what to put
[2:00pm] smorgan: Original author is Xcode?
[2:00pm] sauron: oh, i didn't realize Xcode generated it
[2:00pm] sauron: haha
[2:01pm] sauron: I thought it was Apple sample code like MoreFiles
[2:01pm] sauron: though "Original Author: Xcode 3" sounds kinda fun

We don't have to worry about the original author part now with MPL2, but I still don't know if putting a license header of our own on  Apple-generated code is the "right" thing to do.

Oh, hmm; Apple's example code package for Spotlight importers does have the typical Apple license header for main.c[1], like MoreFiles[2] used to < Gecko 1.9.2, so maybe that's the right license header to use?  OTOH, Google's Apache'd theirs in Google Toolbox for Mac[3]…

[1] http://developer.apple.com/library/mac/#samplecode/Spotlight/Listings/dbg_importer_main_c.html
[2] http://mxr.mozilla.org/mozilla1.9.1/source/xpcom/MoreFiles/MoreFilesX.c
[3] http://google-toolbox-for-mac.googlecode.com/svn/trunk/SpotlightPlugins/Common/main.c

At any rate, armed with that information, you're probably in the best position to determine the right license header for src/spotlight-importer/Importer-main.c ;-)
(In reply to Smokey Ardisson (not following bugs - do not email) from comment #13)
> This has been blocked on real life and releases, and the need to find time
> to clear every Camino off of my Mac in order to make sure the Spotlight
> importer still works with a license header in schema.xml. :-(
> 
> Philippe has volunteered ;-) to test the latter for me, …

After purging the 10.6 machine from all instances of Camino, and installing a patched Camino:
1. Freshly created Bookmarks are found with Spotlight
2. quicklook on those BM's (still) works
3. updating (old/new) bookmarks (adding keyword or description, modifying title of a BM), Spotlight finds the updated items by modified keyword/desc.,etc.

Sounds like everything is peachy.
If it's auto-generated code, we probably don't want to put a license header on it.

If there are a small handful of files whose status is holding up this review, then it would be much easier to just skip them. There's no problem with that. 9/10 of a loaf is better than no bread :-)

Gerv
Comment on attachment 590750 [details] [diff] [review]
Patch v.2

r=ardissone with the following fixed or otherwise explained:

* These two files need to be excluded from having a license added:

IBPalette/resources/palette.table
src/spotlight-importer/Importer-main.c

The former is actually a .plist with another non-.plist extension, and the latter is per the discussion in comment 13/comment 15.

* The following files are missing from the patch and should, in theory, have a license added or upgraded:

feedhandlers/InfoPlist.strings.in
flashblock/Makefile.in
geckochrome/locale/en-US/mozapps/plugins/plugins.dtd
IBPalette/resources/English.lproj/InfoPlist.strings
IBPalette/src/CaminoViewsFramework_Prefix.pch
IBPalette/src/CaminoViewsInspector.h
IBPalette/src/CaminoViewsInspector.m
IBPalette/src/CaminoViewsPalette_Prefix.pch
IBPalette/src/CaminoViewsPalette.h
IBPalette/src/CaminoViewsPalette.m
resources/application/FieldSchema.tbl
resources/application/SchemaStrings.tbl
resources/application/StateSchema.tbl
resources/localized/English.lproj/global_locale_appstrings_properties.strings.in
resources/localized/English.lproj/global_locale_config_properties.strings.in
resources/localized/English.lproj/global_locale_css_properties.strings.in
resources/localized/English.lproj/global_locale_dom_dom_properties.strings.in
resources/localized/English.lproj/global_locale_layout_errors_properties.strings.in
resources/localized/English.lproj/global_locale_layout_HtmlForm_properties.strings.in
resources/localized/English.lproj/global_locale_layout_MediaDocument_properties.strings.in
resources/localized/English.lproj/global_locale_layout_xmlparser_properties.strings.in
resources/localized/English.lproj/global_locale_prompts_properties.strings.in
resources/localized/English.lproj/global_locale_svg_svg_properties.strings.in
resources/localized/English.lproj/global_locale_xslt_xslt_properties.strings.in
resources/localized/English.lproj/InfoPlist.strings.in
resources/localized/English.lproj/necko_locale_necko_properties.strings.in
resources/localized/English.lproj/pipnss_locale_pipnss_properties.strings.in
resources/localized/English.lproj/pipnss_locale_security_properties.strings.in
src/bookmarks/ICabBookmarkConverter.h
src/bookmarks/ICabBookmarkConverter.m

I suspect the InfoPlist.strings(.in) files were caught by an over-zealous "plist" exclusion, but these are actually .strings files.  (Note the non-.in one is UTF-16-encoded).

The flashblock/ Makefile is the only part of the flashblock code that's actually "Camino code" and not from upstream, so it *can* be upgraded to MPL2.

I suspect all the *_properties.strings.in are the ones with the double-quoting from wrapping the Gecko quoting style into .strings quoting style (comment 3/comment 4); that was the case with the couple of them I checked.

The ICabBookmarkConverter files were added since the patch was generated ;)

Thanks for doing this, and sorry for the delay in finishing this.
Attachment #590750 - Flags: review?(alqahira) → review+
OK, I pushed the big change, having reverted the two excluded files, and relicensed one or two of your big list. I then did the rest in a follow-up push.

Gerv
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Looks like

resources/application/FieldSchema.tbl
resources/application/SchemaStrings.tbl
resources/application/StateSchema.tbl

got missed.

Like some of the other projects, I'm going to move this bug into our product.
Component: Licensing → General
Product: mozilla.org → Camino
Whiteboard: [camino-2.1.3]
Version: other → unspecified
Smokey: yes, I wasn't sure about the comment char for those 3 files. Feel free to do them manually :-).

Gerv
(In reply to Gervase Markham [:gerv] from comment #19)
> Smokey: yes, I wasn't sure about the comment char for those 3 files. Feel
> free to do them manually :-).

When I tested, I used the same license block style as the other .tbl files, so that's what I pushed.

For the record, commits for this bug were:

http://hg.mozilla.org/camino/rev/43d400c13b9d (Gerv's big commit)
http://hg.mozilla.org/camino/rev/d2f5fca4b8fb (Gerv's follow-up commit)
http://hg.mozilla.org/camino/rev/b13aa410b25a (my commit for the 3 .tbl files)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: