Remove installchrome.pl and installcfunc.pl

RESOLVED FIXED in mozilla7

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
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.
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!
(Assignee)

Comment 3

6 years ago
(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 :)
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.
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
Created attachment 538520 [details] [diff] [review]
Remove installchome.pl and installcfunc.pl
(Assignee)

Updated

6 years ago
Attachment #538520 - Flags: review?(khuey)
Attachment #538520 - Flags: review?(benjamin)
Comment on attachment 538520 [details] [diff] [review]
Remove installchome.pl and installcfunc.pl

Only one review is necessary.
Attachment #538520 - Flags: review?(khuey)
Attachment #538520 - Flags: review?(benjamin)
Attachment #538520 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → joey
Summary: mozilla-central/config/installcfunc.pl enhancements → Remove installchrome.pl and installcfunc.pl
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a59f48381ad
Keywords: checkin-needed
Whiteboard: [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/3a59f48381ad
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
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!
You need to log in before you can comment on or make changes to this bug.