Last Comment Bug 662796 - Remove installchrome.pl and installcfunc.pl
: Remove installchrome.pl and installcfunc.pl
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 07:35 PDT by Joey Armstrong [:joey]
Modified: 2011-08-26 01:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove installchome.pl and installcfunc.pl (8.24 KB, patch)
2011-06-10 08:23 PDT, Joey Armstrong [:joey]
benjamin: review+
Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2011-06-08 07:35:48 PDT
1) A make driven unit test should be written for the script to verify existing functionality.

2) Convert script into a .pm module rather than calling "return(1);" to simulate the behavior.  This is likely legacy code used for importing modules with require, foo.pm + "use module" is the newer form.  installcfunc.pl can remain but it should become a very thin wrapper.

3) Add "use strict; use warnings;" (-vs- perl -w)

4) Replace shell globbing "<$inPath${gPathDelimiter}>" with opendir() or one of the newer 5.10.x modules with builtin support for shell pattern globbing.  Forking a shell to gather files can fail when build machines are under a heavy load or are unable to spawn new processes.

5) String & regex comparisons could be replaced with hashes for optimization and to greatly reduce the code footprint.  A unit test will need to be setup before this is attempted.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-08 07:44:03 PDT
I think this is dead code.  installcfunc.pl appears to only be used from installchrome.pl, which appears to be unused.  This was probably replaced with python many moons ago.
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-08 08:04:11 PDT
In fact, I have never heard of this script, and I've been in charge of the chrome install scripts since 2003. Rip it out!
Comment 3 Joey Armstrong [:joey] 2011-06-10 07:30:02 PDT
(In reply to comment #2)
> In fact, I have never heard of this script, and I've been in charge of the
> chrome install scripts since 2003. Rip it out!

This sounds like a relatively safe edit then :)
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-06-10 07:55:41 PDT
I think his point was that we'd just like a patch to "hg rm" the files since they're unused. No point in cleaning up the trash.
Comment 5 Joey Armstrong [:joey] 2011-06-10 08:12:30 PDT
(In reply to comment #4)
> I think his point was that we'd just like a patch to "hg rm" the files since
> they're unused. No point in cleaning up the trash.

Yes understood, removal cannot be much "safer" than when the script has atrophied to the point that a maintainer is not familiar with the code.
Comment 6 Joey Armstrong [:joey] 2011-06-10 08:23:12 PDT
Created attachment 538520 [details] [diff] [review]
Remove installchome.pl and installcfunc.pl
Comment 7 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-10 09:11:15 PDT
Comment on attachment 538520 [details] [diff] [review]
Remove installchome.pl and installcfunc.pl

Only one review is necessary.
Comment 8 Phil Ringnalda (:philor, back in August) 2011-06-18 19:58:35 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a59f48381ad
Comment 9 Mounir Lamouri (:mounir) 2011-06-19 04:06:51 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/3a59f48381ad
Comment 10 Simona B [:simonab] 2011-08-26 01:49:33 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

I checked that the files were removed from the repo:
http://hg.mozilla.org/mozilla-central/file/cc1e08803869/config

Is there a test case to verify this manually or is this enough to mark this as Verified Fixed?

Thanks!

Note You need to log in before you can comment on or make changes to this bug.