Bug 66754 - Remove own implementations of various Perl functions in installer
Summary: Remove own implementations of various Perl functions in installer
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyMedium, easyHack, skillPerl, topicCleanup
Depends on:
Blocks: Installer
  Show dependency treegraph
 
Reported: 2013-07-10 06:32 UTC by Jan Holesovsky
Modified: 2022-05-02 14:40 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Switch mkdir to make_path (5.48 KB, patch)
2020-03-07 12:02 UTC, ed-documentfoundation
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Holesovsky 2013-07-10 06:32:43 UTC
Some Perl knowledge is needed for this Easy Hack.

solenv/bin/modules/installer/systemactions.pm implements for example create_directory() that can be exchanged with standard Perl's make_path() [http://perldoc.perl.org/File/Path.html].  Similarly other functions there, for which standard (or widely used) implementations exist.

What needs to be done (in the create_directory case; but similarly for more functions there):

Find all Perl scripts that use create_directory:

git grep '\<create_directory\>'

Edit all those that are Perl (end with .pl or .pm) so that you add use File::Path qw(make_path); at the top and then instead of create_directory() calls, you use make_path().

Then, remove the create_directory and create_directory_with_privileges declarations / definitions.

As the last step, make sure that everything builds & packages still :-)
Comment 1 Björn Michaelsen 2013-10-04 18:46:38 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 2 Robinson Tryon (qubit) 2013-10-23 17:24:21 UTC Comment hidden (obsolete)
Comment 3 bavincen 2014-09-23 06:26:58 UTC
(In reply to comment #0)
> Some Perl knowledge is needed for this Easy Hack.
> 
> solenv/bin/modules/installer/systemactions.pm implements for example
> create_directory() that can be exchanged with standard Perl's make_path()
> [http://perldoc.perl.org/File/Path.html].  Similarly other functions there,
> for which standard (or widely used) implementations exist.
> 
> What needs to be done (in the create_directory case; but similarly for more
> functions there):
> 
> Find all Perl scripts that use create_directory:
> 
> git grep '\<create_directory\>'
> 
> Edit all those that are Perl (end with .pl or .pm) so that you add use
> File::Path qw(make_path); at the top and then instead of create_directory()
> calls, you use make_path().
> 
> Then, remove the create_directory and create_directory_with_privileges
> declarations / definitions.
> 
> As the last step, make sure that everything builds & packages still :-)

Hi jan
can we keep that  create_directory and implement make path in it.
and remove  create_directory_with_privileges ,

need a breif explanation :) thanks
Comment 4 Taylor Lee 2015-06-10 07:12:11 UTC
bavincen,

I think your solution is a step in the right direction.  I find 'sub create_directory' in the following 4 files:

 setup_native/scripts/admin.pl
 solenv/bin/modules/installer/systemactions.pm
 solenv/bin/modules/par2script/systemactions.pm
 solenv/bin/modules/pre2par/systemactions.pm

The last 2 files don't seem to need make_path as there is no implementation of recursive directory creation; mkdir seems sufficient.

In the first 2 files, I think it makes sense to replace the contents of create_directory() with make_path and some error detection as was implemented in create_directory_with_privileges().

I argue that there are many calls to installer::systemactions::create_directory($var), so does changing all calls to "make_path($var, {mode=0775})" make sense?  Maybe having mode in a central function is a good thing?  Not sure.

> egrep -ciIR installer::systemactions::create_directory | egrep -v ':0$'
solenv/bin/modules/installer.pm:6
solenv/bin/modules/installer/parameter.pm:2
solenv/bin/modules/installer/worker.pm:6
solenv/bin/modules/installer/windows/mergemodule.pm:3
solenv/bin/modules/installer/windows/idtglobal.pm:2
solenv/bin/modules/installer/windows/strip.pm:1
solenv/bin/modules/installer/windows/update.pm:1
solenv/bin/modules/installer/windows/msp.pm:7
solenv/bin/modules/installer/windows/admin.pm:2
solenv/bin/modules/installer/strip.pm:1
solenv/bin/modules/installer/profiles.pm:1
solenv/bin/modules/installer/simplepackage.pm:9
solenv/bin/modules/installer/archivefiles.pm:2
solenv/bin/modules/installer/scpzipfiles.pm:2
solenv/bin/modules/installer/download.pm:1
solenv/bin/modules/installer/copyproject.pm:1
Comment 5 Robinson Tryon (qubit) 2015-12-14 07:10:42 UTC Comment hidden (obsolete)
Comment 6 Josh Baldwin 2016-01-26 02:18:18 UTC
Hello, I would like to take up this task as my first LO commit.  I will start with just one file though, to make sure I understand the process.  Then I will change all create_directory() to make_path().  After that, I can research more functions we can move into the core and leave those findings here for more new developers.

Question though for Taylor:
>I argue that there are many calls to >installer::systemactions::create_directory($var), so does changing all calls to >"make_path($var, {mode=0775})" make sense?  Maybe having mode in a central >function is a good thing?  Not sure.

I do follow your questions.  Can you explain your thought process in more detail?  Thanks!
Comment 7 Jan Holesovsky 2016-01-26 08:01:23 UTC
Josh: Great to see you hacking on this!  Let me CC JanI who will be able to help you with pushing your patches etc. :-)
Comment 8 jani 2016-01-28 08:00:51 UTC
(In reply to Jan Holesovsky from comment #7)
> Josh: Great to see you hacking on this!  Let me CC JanI who will be able to
> help you with pushing your patches etc. :-)

Yes I am here to help non-committers getting their first patch submitted.

if you have problems/questions please feel free to email me directly 
jani@documentfoundation.org

Of also using IRC and our mailing list is a good idea.

We have created a step by step guide, to help you over the first steps:
https://wiki.documentfoundation.org/Development/GetInvolved/DeveloperStepByStep

have fun
Comment 9 Josh Baldwin 2016-01-31 14:03:24 UTC
I am having email issues, so I'll post this here:

I made the change to /setup_native/scripts/admin.pl.  I did

egrep -ir "admin.pl" .

to see where in libreoffice admin.pl used.  However, it does not appear to be called anywhere.  Any ideas on how this file is used? Is it manually called and not from any other part of libreoffice?  I wanted to know so I can properly test the code before submitting.  Any insight will be greatly appreciated.
Comment 10 Christian Lohmaier 2016-02-01 09:53:37 UTC
just read the file and you'll see that's it can be used to do a special kind of installation using the created msi installer packages. It is not used during building of LO.
Comment 11 jani 2016-04-18 07:30:38 UTC
A polite ping, still working on this issue?
Comment 12 jani 2016-06-03 06:02:57 UTC
Unassigning due to lack of work, if you still work on the issue, please assign it again.
Comment 13 ed-documentfoundation 2020-03-07 12:02:28 UTC
Created attachment 158465 [details]
Switch mkdir to make_path

Switch mkdir to make_path.

Since the build can run on several processors at once, mkdirs may run into themselves. It makes sense to keep create_directory code but replace the mkdirs. make_path only returns the directories which it created, two running at once could be likely on a busy file system.
Comment 14 Buovjaga 2020-03-07 12:34:54 UTC
(In reply to ed-documentfoundation from comment #13)
> Created attachment 158465 [details]
> Switch mkdir to make_path
> 
> Switch mkdir to make_path.
> 
> Since the build can run on several processors at once, mkdirs may run into
> themselves. It makes sense to keep create_directory code but replace the
> mkdirs. make_path only returns the directories which it created, two running
> at once could be likely on a busy file system.

Please submit the patch to gerrit:
https://wiki.documentfoundation.org/Development/gerrit

Alternative way is to recreate it directly inside gerrit:
https://wiki.documentfoundation.org/Documentation/GerritEditing
Comment 15 ed-documentfoundation 2020-03-07 16:25:07 UTC
Thanks!

It tells me it is uploaded here:

  <https://gerrit.libreoffice.org/c/core/+/90166>

Out of interest, how should I link the commit with this bug? In GitLab/GitHub parlance Closes/Fixes terms in the commit or PR do the linking.
Comment 16 Buovjaga 2020-03-07 17:49:37 UTC
(In reply to ed-documentfoundation from comment #15)
> Thanks!
> 
> It tells me it is uploaded here:
> 
>   <https://gerrit.libreoffice.org/c/core/+/90166>
> 
> Out of interest, how should I link the commit with this bug? In
> GitLab/GitHub parlance Closes/Fixes terms in the commit or PR do the linking.

Ah, sorry for not mentioning: make the first line of the commit message begin with

tdf#66754

Click the "EDIT" text under the commit message in gerrit to modify it.
Comment 17 Xisco Faulí 2020-07-22 15:10:10 UTC
Dear ed-documentfoundation@s5h.net,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assign it back to yourself if you're still working on this.
Comment 18 Xisco Faulí 2022-05-02 14:40:09 UTC
Dear Jason,
This bug has been in ASSIGNED status for more than 3 months without any
activity. Resetting it to NEW.
Please assign it back to yourself if you're still working on this.