patchGNU Octave - Patches: patch #7976, Make PPKG_ADD and PKG_DEL see both...

 
 

patch #7976: Make PPKG_ADD and PKG_DEL see both m-file and oct-file directories of a package (2nd post)

Submitter:  Olaf Till <i7tiol>
Submitted:  Wed 20 Mar 2013 11:28:22 PM UTC
   
 
Category:  Core : other Priority:  3 - Low
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 18 Jan 2020 05:11:58 PM UTC, comment #10: 

Revisit for Octave 6.

No real progress on this item for almost 7 years.  Patches do no longer apply cleanly.  I think the pkg tool today is totally different to that of 7 years ago.

https://hg.savannah.gnu.org/hgweb/octave/log/27a5cc0614f1/scripts/pkg/pkg.m

Thus I do not expect much value from this patch anymore.

Closing item.

Kai Torben Ohlhus <siko1056>
Group Member
Wed 06 Jul 2016 01:06:39 PM UTC, comment #9: 

Jordi, should this patch be considered for 4.2.0?

Lachlan Andrew <lachlan>
Tue 23 Jun 2015 12:08:32 PM UTC, comment #8: 

Related: bug #45369

Julien Bect <jbect>
Fri 22 Mar 2013 09:22:44 PM UTC, comment #7: 

The reallocs are actually slow enough, slower than std::list, but the difference only matters for some very giant datasets:

http://www.baptiste-wicht.com/2012/11/cpp-benchmark-vector-vs-list/

Like I said originally, I agree with you, I don't think this really matters too much.

jwe said he wanted to look over your patch, so when he oks it, I'll push it, if he doesn't do it himself first.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 22 Mar 2013 09:11:19 PM UTC, comment #6: 

They reallocate, but contrary to lists they preallocate space for many elements at once, so new (re)allocations are seldom needed. This makes them more efficient than lists in adding elements to the end, at least this is how I understand an explanation in www.cplusplus.com.

I don't think it's important enough in this case to profile which is better. If you believe otherwise, feel free to change to std::list.

Olaf Till <i7tiol>
Group Member
Fri 22 Mar 2013 07:52:58 PM UTC, comment #5: 

Adding elements to the end of an std::vector is precisely the situation for which std::vector is worse than std::list, since this requires reallocs.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 22 Mar 2013 07:51:12 PM UTC, comment #4: 

I have revised the patch without applying the suggestion of the last post. Changes (suggested by Jordi):

- merged both patches

- shortened first line of commit message

- corrected the typos

- deleted commented out code

I did not use typedefs for the long iterator names since all over the file there are already similar names without typedefs. The choice of std::vector was commented in the last post.

(file #27661)

Olaf Till <i7tiol>
Group Member
Fri 22 Mar 2013 07:40:15 AM UTC, comment #3: 

As for the architecture issue, as I see it for the case of adding paths:

1st level: functions (without '::do_') defined in load-path.h, taking care only of instantiation;

2nd level: most functions with '::do_' in load-path.cc;

3rd level: ::do_add () in load-path.cc.

3rd level does actual path adding. But it also calls the add_hook.

2nd level calls 3rd level for path adding. But some functions in 2nd level want to call add_hook in their own way. So they have to temporarily set add_hook to zero and protect it while calling 3rd level. Also, some functions in 2nd level call 3rd level indirectly over a further 2nd level function (which has not much to do).

Suggestion: Let all 2nd level functions call 3rd level directly. Don't let 3rd level call the add_hook. This spares disabling it in the 2nd level. Let all 2nd level functions call the add_hook themselves; the only function which currently doesn't is ::do_append.

For the case of removing paths it's similar, but in one case 1st level ('remove()') calles 3rd level (::do_remove) directly. So for a change analog to adding paths one would have to introduce a short new function for the 2nd level here.


As for the choice of std::vector, it was because elements are only added to the end, which should be more efficient than with std::list.

Olaf Till <i7tiol>
Group Member
Thu 21 Mar 2013 05:21:14 PM UTC, comment #2: 

jordigh wrote:

I'm not entirely happy with the whole architecture of this patch, but I'm not entirely happy with the organisation of this whole load-path architecture either. It feels to me like we're adding wrapper functions on top of wrapper functions without fixing the actual underlying architectural problem.

What do you think the underlying problems are?

Maybe we should discuss this on the maintainers list before making changes?  At least we should agree on what load-path should do and determine what it is not currently doing correctly.

John W. Eaton <jwe>
Group administrator
Thu 21 Mar 2013 05:10:00 PM UTC, comment #1: 

Your two csets depend on each other. Indeed, you'll see that the parent of pkg-addpath-rmpath.changeset is addpath-rmpath.changeset. In situations like these, it's best if you concatenate both into the same file. Here "hg export -r -2:-1" would do it, assuming that -2 and -1 point to these two commits (i.e. if they're the last two chronologically on your repository).

Your commit message for addpath-rmpath.changset has long lines. The first line should be short, under 80 characters.

Typo:


(rmpath): fill vector of direcories and call load_path::rm_dirlst.


"direcories"?


+
+  for (std::vector<std::string>::const_iterator i = dirlst.begin ();
+       i != dirlst.end (); i++)
+    do_add (*i, at_end, warn);


We have some typedefs for those long iterator names, but why are you using std::vector instead of std::list? You don't need random access on this directory list. For the usual sizes of these lists, this difference in the choice of container doesn't matter much, granted.


                  std::string dir = *p;
-
+                  dirlst.push_back (*p);
+
+                  //std::string dir = *p;
                   //dir = regexprep (dir_elts{j}, '//+', "/");
                   //dir = regexprep (dir, '/$', "");


Why are you leaving commented-out code? If the code needs to be deleted, banish it to VCS history, not to a comment.

I'm not entirely happy with the whole architecture of this patch, but I'm not entirely happy with the organisation of this whole load-path architecture either. It feels to me like we're adding wrapper functions on top of wrapper functions without fixing the actual underlying architectural problem.

Your second patch looks good. Small typo:


+  if (iscellstr (out_file))
+    if ((n = numel (out_file)) != numel (directory))
+      error ("number of output files must be one ore matching number of directories");
+    endif


"ore"?

I recommend using Mercurial's histedit extension for polishing these two patches. Briefly, after enabling the histedit extension, do


hg histedit -2


and this should open an editor where you can specify what to do to your last two revisions. Select "edit" for both of them, close the editor, and edit each revision in turn, doing


hg histedit --continue


when you're done with each one.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Wed 20 Mar 2013 11:28:22 PM UTC, original submission:  

sorry, in previous post I accidentally hit Enter

Problem:

PKG_ADD and PKG_DEL instructions may need access to both the architecture-dependent and the architecture-independent directories of a package. But as yet they are added to or removed from the path sequentielly, thereby separately calling PKG_ADD or PKG_DEL, respectively. In 'pkg build' they were even (added/removed) separately for each directory by generate_lookfor_cache.m/gen_doc_cache.m.

This recently caused problems for the database package in 'pkg build'. Reportedly there were also problems with other packages due to it. These problems have been worked around in the packages. But the fix in Octave is not difficult:

The first patch slightly modifies load-path.cc/h to make 'addpath' call the hook for the PKG_ADDs only after all argument directories are added (similarly as the ::do_set method already did it), and correspondingly to 'rmpath'.

The second patch changes some functions called by pkg.m to make use of the feature in the first path by always calling addpath and rmpath once for all directories.

I made a testpackage with PKG_ADD and PKG_DEL instructions as described above which yielded errors for 'pkg build', 'pkg unload', and 'pkg uninstall', and no errors with the patches. (testpackage not attached)

Olaf

Olaf Till <i7tiol>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #27661:  revised-addpath-rmpath.changeset added by i7tiol (14KiB - application/octet-stream)
file #27643:  addpath-rmpath.changeset added by i7tiol (7KiB - application/octet-stream)
file #27644:  pkg-addpath-rmpath.changeset added by i7tiol (7KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by jbect (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by i7tiol (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-25 mtmiller Carbon-CopyRemoved 80942 -
    2020-01-18 siko1056 Priority5 - Normal 3 - Low
        StatusIn Progress Wont Do
        Assigned tojordigh None
        Open/ClosedOpen Closed
    2018-04-10 mtmiller CategoryNone Core : other
    2013-03-22 i7tiol Attached File- Added revised-addpath-rmpath.changeset, #27661
    2013-03-21 jordigh StatusNone In Progress
        Assigned toNone jordigh
    2013-03-20 i7tiol Attached File- Added addpath-rmpath.changeset, #27643
        Attached File- Added pkg-addpath-rmpath.changeset, #27644

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code