bugGNU Octave - Bugs: bug #45369, pkg build leads to duplication of...

 
 

bug #45369: pkg build leads to duplication of PKG_ADD and PKG_DEL files

Submitter:  Carnë Draug <carandraug>
Submitted:  Sun 21 Jun 2015 11:55:19 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 16 May 2016 12:28:16 PM UTC, comment #33: 


> What is the status of this bug report?
>
> Is it solved? Does it need more testing?


I think it is solved. Closing as such.

Carnë Draug <carandraug>
Group Member
Mon 16 May 2016 11:17:25 AM UTC, comment #32: 

What is the status of this bug report?

Is it solved? Does it need more testing?

Julien Bect <jbect>
Wed 23 Mar 2016 01:43:29 AM UTC, comment #31: 

Marco, thank you for testing this. I'm mostly worried about breaking stuff in downstream packagers.

> The change ported to 4.0.1 works fine.


For those reading it here, it works in 4.0.1 because Marco backported 2ee20a290d61 and 8e5eca2c5a64 into 4.0.1 for the cygwin releases.

> However the location of PKG_ADD and PKG_DEL is depending on the specific package:


The final location is supposed to be dependent. Some packages need it in the arch dependent directory, others in the arch independent, other need different PKG_ADD in each directory.

Here's what is supposed to happen on each of them:

  • io: you should have a PKG_ADD and PKG_DEL file in the arch independent directory.
  • communications: you should have a PKG_ADD and PKG_DEL in the arch dependent directory
  • ltfat: this is a very special case because of their multi-directory structure. Still, the files are going in the right place for you.


This matches your results after applying the patch which is great.

Carnë Draug <carandraug>
Group Member
Tue 22 Mar 2016 11:02:43 PM UTC, comment #30: 

The change ported to 4.0.1 works fine.

However the location  of PKG_ADD and PKG_DEL is depending on the specific package:
on "io" I see a difference, communication does not change, ltfat
maintain its anomaly.

before :

$ cygcheck -l octave-ltfat |grep PKG
/usr/lib/octave/packages/ltfat-2.1.2/x86_64-unknown-cygwin-api-v50+/PKG_ADD
/usr/lib/octave/packages/ltfat-2.1.2/x86_64-unknown-cygwin-api-v50+/PKG_DEL
/usr/share/octave/packages/ltfat-2.1.2/ltfat/PKG_ADD

$ cygcheck -l octave-io |grep PKG
/usr/lib/octave/packages/io-2.4.1/x86_64-unknown-cygwin-api-v50+/PKG_ADD
/usr/lib/octave/packages/io-2.4.1/x86_64-unknown-cygwin-api-v50+/PKG_DEL

$ cygcheck -l octave-communications |grep PKG
/usr/lib/octave/packages/communications-1.2.1/x86_64-unknown-cygwin-api-v50+/PKG_ADD
/usr/lib/octave/packages/communications-1.2.1/x86_64-unknown-cygwin-api-v50+/PKG_DEL

After:
cygport octave-ltfat.cygport list |grep PKG
  /usr/lib/octave/packages/ltfat-2.1.2/x86_64-unknown-cygwin-api-v50+/PKG_ADD
  /usr/lib/octave/packages/ltfat-2.1.2/x86_64-unknown-cygwin-api-v50+/PKG_DEL
  /usr/share/octave/packages/ltfat-2.1.2/ltfat/PKG_ADD

$ cygport octave-io.cygport list |grep PKG
  /usr/share/octave/packages/io-2.4.1/PKG_ADD
  /usr/share/octave/packages/io-2.4.1/PKG_DEL

$ cygport octave-communications.cygport list |grep PKG
  /usr/lib/octave/packages/communications-1.2.1/x86_64-unknown-cygwin-api-v50+/PKG_ADD
  /usr/lib/octave/packages/communications-1.2.1/x86_64-unknown-cygwin-api-v50+/PKG_DEL


(file #36730)

marco atzeri <matzeri>
Tue 22 Mar 2016 03:18:48 AM UTC, comment #29: 

I pushed the change below (after a few fixes) which fixes the issue by redefining what an Octave binary package is.

I have tested in a couple of different packages and works for me.

Carnë Draug <carandraug>
Group Member
Mon 21 Mar 2016 03:51:30 AM UTC, comment #28: 

I am attaching a patch that addresses the issue with "pkg build" only.

pkg build works by installing the package in a specific place and the making a tarball of that.  This causes several problems with PKG_ADD and PKG_DEL (note that it is not possible to have a PKG_ADD file within inst -- see bug #47481). What is doing at the moment is moving the PKG_ADD file that is installed back to the root of the package to create a binary release of the package.

This patch changes the whole logic of "pkg build".  It simply runs configure make within src, and then packages everything again.  This fixes second guessing the origin of PKG_ADD files.  It also future proofs for when the architecture dependent directory is no longer nested within the package architecture dependent directory.

It mades two nice additions inspired by python's wheel pep. It explicitly mentions the Octave abi version and arch for the release.

Would be nice if anyone that makes uses of pkg build could test this.

(file #36706)

Carnë Draug <carandraug>
Group Member
Thu 09 Jul 2015 10:17:06 AM UTC, comment #27: 


> > Will it be possible to have PKG_ADD directives which
> > depend on directories of other packages being in the
> > path if these other packages are named in the same
> > 'pkg install ... ...' command?


> Could you describe a specific use case for this ?


I currently don't know an example.

> Would it be enough to know that dependencies are always loaded > before the package that depends on them (I don't know if it is > the case currently) ?


If there are no circular dependencies, it's enough.

If there are circular dependencies, first the directories of all packages named in 'pkg install ... ...' must be added to the path before the PKG_ADD instructions of any of these packages are executed.

Why should we disallow circular package dependencies if we are able to allow them if these packages are installed in one pkg call.

Olaf Till <i7tiol>
Group Member
Thu 09 Jul 2015 09:03:14 AM UTC, comment #26: 


> Will it be possible to have PKG_ADD directives which
> depend on both the arch-dependent and -independent
> directories being in the path?


Yes, with my solution you can do that. See Comment #24, item 5: the order in which the two directories are addpath-ed would be specified by the documentation. Therefore, you could rely on this information to write your PKG_ADD directives in the PKG_ADD file located in the directory that is loaded second.


> Will it be possible to have PKG_ADD directives which
> depend on directories of other packages being in the
> path if these other packages are named in the same
> 'pkg install ... ...' command?


Could you describe a specific use case for this ?

Would it be enough to know that dependencies are always loaded before the package that depends on them (I don't know if it is the case currently) ?

Julien Bect <jbect>
Thu 09 Jul 2015 08:09:05 AM UTC, comment #25: 

Will it be possible to have PKG_ADD directives which depend on both the arch-dependent and -independent directories being in the path?

Will it be possible to have PKG_ADD directives which depend on directories of other packages being in the path if these other packages are named in the same 'pkg install ... ...' command?

Olaf Till <i7tiol>
Group Member
Thu 09 Jul 2015 07:46:59 AM UTC, comment #24: 

Here is a new version of the approach proposed in Comment #8.

-----

1) No longer accept PKG_ADD/PKG_DEL files at the root of the package tarball.

1.1) Transition period: if there is a PKG_ADD/PKG_DEL file at the root of the package and none in ./src (for source tarballs) or ./inst/$ARCH (for binary tarballs), warn about deprecated package structure and move the files to the arch-dependent directory. (Remark: currently, it seems assumed that packages with a PKG_ADD/PKG_DEL file must have an arch-dependent directory. I would change that and move the files to the arch-independent directory if there is nio arch-dependent.)

1.2) Later: after the transition period, turn the warning into an error.

-----

2) Accept PKG_ADD/PKG_DEL files in ./inst, or in any subdirectory of ./inst.

2.1) Those files are copied at the corresponding location in the arch-independent directory. This means that pkg must copy PKG_ADD/PKG_DEL files (or all files ?) together with m-files. (I believe that the current behaviour is to ignore non-m-files, but I couldn't find the place where this happens in the code.)

2.2) When a package is loaded, only the top-level PKG_ADD file is executed, when the directory is addpath-ed. The other ones are not executed automatically. They will be executed, typically, when the top-level PKG_ADD file adds the subdirectories to the path (if it does). And similarly for PKG_DEL.

-----

3) Also, accept PKG_ADD/PKG_DEL files in ./src (for source packages) or in ./inst/ARCH-NAME (for binary packages). Those files are copied in the arch-dependent directory.

3.1) Note that ./src and ./inst/ARCH-NAME should never exist simultaneously, since the second one is created by pkg build, which removes src.




4) Keep accepting PKG_ADD/PKG_DEL directives (of course)

4.1) Note: I think that PKG_ADD/PKG_DEL directives should also be accepted in subdirectories, for consistency. Currently they are ignored.

4.2) PKG_ADD/PKG_DEL directives located in m-files (in ./inst or subdirectories of ./inst) must be removed (or disabled) from theses files after having been processed. This will prevent them from being processed twice in the case of "pkg build" followed by "pkg install". There is no such problem with C++ source files, since the src directory is not included in the binary tarball by "pkg build".

4.3) Note: 4.2 is slightly different from the solutions discussed in previous comments. But this seems to be the lightest solution to implement, since "pkg build" actually installs the package using the same install () subfunction as "pkg install" and then creates a binary tarball using the repackage () subfunction.

-----

5) To make things perfectly clear for package developpers, specify in the documentation in which order the two directories (on platforms where they are separate) are addpath-ed/rmpath-ed at package load/unload time, and therefore in which order the corresponding PKG_ADD/PKG_DEL files are executed.

-----

6) Process demos and test blocks from C++ source files and install them in the arch-independent directory.

6.1) I haven't checked the solution proposed in bug reports #41298 and #41215, but I assume this is what they do.

6.2) We must ensure that this plays well with the "pkg build" / "pkg install" combo, i.e., that the corresponding files (*.tst or whatever) are properly repackaged (copied from the arch-independent dir to ./inst in the binary tarball, for instance) and then installed (i.e. copied back from ./inst to the arch-independent directory).

-----

What do you think ?

Julien Bect <jbect>
Thu 09 Jul 2015 05:06:07 AM UTC, comment #23: 

Thank you Mike for the reference to bug #41298.

Another related bug is bug #41215.

I also find it more natural to extract everything during the build (or the first part of install) and then delete src.

I will try (again) to propose a full sketch of a solution later.

Julien Bect <jbect>
Wed 08 Jul 2015 09:10:53 PM UTC, comment #22: 

For the issue of demos and tests, please refer to bug #41298 (which has an as-yet unreviewed patch). I think the idea there is to extract tests and demo blocks to separate files, following what Octave does in its own build.

If that is done, there's no need to keep source files just to have demos and tests available. I would lean towards having the `pkg build` or `pkg install` step extract everything from source files that needs to be kept around and delete the full source.

Mike Miller <mtmiller>
Group Member
Wed 08 Jul 2015 09:02:11 PM UTC, comment #21: 


> There is another issue related it. demos and tests
> from functions in src/ are lost during install because
> the source files are removed after building. If they are
> kept around for this purpose, they would still be available
> in binary distributions. This would also make them available
> for the binary installs.


The only think that make a "source tarball" different form a "binary tarball", currently, is that ./src has been removed from the second one. This has the important consequence that configure_make() doesn't try to build a second time.

So, if you decide to keep ./src in binary tarballs, it means that pkg () will have to distinguish otherwise between source and binary tarballs. Not so hard, though: we could decide that the configure/make step is skipped if the arch-dependent directory is already present inside ./inst (that's where it is in binary tarballs).

Now, assuming that ./src is not removed directly after building, what exactly would you like to do with demos/tests blocks from C++ source files? Do you also intend to install the content of src, or do you intend to extract those blocks and store them somewhere?

Julien Bect <jbect>
Wed 08 Jul 2015 08:45:06 PM UTC, comment #20: 


>> 5) When building a binary package, copy PKG_ADD/PKG_DEL
>> directives into PKG_ADD/PKG_DEL files (this is what we
>> already do) but leave the resulting files where they are
>> (in particular, do not move them at the root of the tarball)
>>
>> 6) When installing a binary package, do not copy
>> PKG_ADD/PKG_DEL directives into PKG_ADD/PKG_DEL files a
>> second time (once is enough !)
>
> This requires pkg() knowing if it's installing a binary
> package or not and adds extra logic dependent on the type
> of installation. Ideally, we would keep differences to
> the minimum. Maybe the opposite is better, to not have
> binary distributions handling PKG_ADD at all, and leave
> that job for the actual pkg install.
> Problem is, a binary install no longer has the source.


You're right, my initial idea doesn't work well.

What about this:

1) When building a binary package, only process PKG_ADD/PKG_DEL directives located in C++ source files.

2) When installing a package, process all directives found in the package.

If the src directory is removed form the binary tarball (as is currentl done) then I think this prevents directives from being processed twice, doesn't it ?

With this approach, 'pkg install' doesn't need to know the difference between source and binary tarballs.

Julien Bect <jbect>
Wed 08 Jul 2015 08:28:40 PM UTC, comment #19: 

Julien Bect said on comment #17:

>> Yes. This is exactly what I proposed. Point is, I can't
>> see a user-case where it makes sense to have the same
>> PKG_ADD and PKG_DEL scripts in both arch-dependent and
>> independent directories.
>
> I never said anything about having the same PKG_ADD / PKG_DEL file in several places. (Did I ?)


No. But currently that is the side-effect of having the file at the root of the package. This could change so that a file at the root of the package gets moved into a specific directory but if we do that, it's just simpler to ask the package developer to do it.

Carnë Draug <carandraug>
Group Member
Wed 08 Jul 2015 08:15:34 PM UTC, comment #18: 


>> 2) Accept PKG_ADD/PKG_DEL files in ./inst, or in any
>> subdirectory of ./inst. Those files are copied at the
>> corresponding location in the arch-independent directory.
>
> Not sure if I understood all the implications of it correctly,
> but I kind of agree. Agree with accepting files in ./inst and
> its subdirectories (seems like pkg() removes non .m files,
> that should not happen). Do not agree on having PKG_ADD on
> subdirectories executed when loading the package. It should
> be the PKG_ADD at the root of inst/ that is responsible for
> calling the others (most likely by adding the subdirectories
> to the path).


I agree: a PKG_ADD in a subdirectory should not be executed automatically. It is executed if the subdirectory is add to the path, which is typically done by the parent PKG_ADD.

I also agree that this implies a change in pkg: non-m-files should not be discarded by pkg (or at least PKG_ADD/PKG_DELL files should not be discarded).

Julien Bect <jbect>
Wed 08 Jul 2015 08:08:54 PM UTC, comment #17: 


>> I just realized that when you said "I think the best
>> fix is to no longer accept PKG_ADD and PKG_DEL files
>> at the root of the package" (see #1), perhaps did you
>> actually mean "not at the root of the package, ./inst
>> and ./src instead" ? In which case this is very close
>> to my proposition.
>
> Yes. This is exactly what I proposed. Point is, I can't
> see a user-case where it makes sense to have the same
> PKG_ADD and PKG_DEL scripts in both arch-dependent and
> independent directories.


I never said anything about having the same PKG_ADD / PKG_DEL file in several places. (Did I ?)

What I really mean is, I think, the same as you:

If there is a PKG_ADD/PKG_DEL file in ./inst, or PKG_ADD/PKG_DEL directives in the m-files that are contained in inst, they should end up in the arch-independent directory.

If there is PKG_ADD/PKG_DEL file in ./src, or PKG_ADD/PKG_DEL directives in the source files that is contained in ./src, they should end up in the arch-dependent directory.

These two options do not have to be mutually exclusive in my opinion. For instance, a package could have PKG_ADD directives in ./inst that call some "setup" functions (see, e.g., stk_init in the stk_package) and have PKG_ADD directives in the src directory (for instance autoload statements). In this case I find more natural two keep two separate PKG_ADD files.

Julien Bect <jbect>
Wed 08 Jul 2015 07:46:51 PM UTC, comment #16: 

@Carnë,

I think that we essentially agree on everything.

I will answer specific points raised by your message (and perhaps raise new questions...) in separate messages.

Julien Bect <jbect>
Wed 08 Jul 2015 07:34:37 PM UTC, comment #15: 

To ease discussion of this, I think there's an important point to be addressed:

== Important point ==

Despite the name, PKG_ADD and PKG_DEL scripts are not scripts that are meant to be run when a package is loaded or unloaded.  They are scripts that are run when a specific directory is added or removed from the path. It is not pkg that handles execution of those files.  Note that Octave itself uses those files. Simply starting octave will execute a bunch of them:


/usr/local/lib/octave/4.0.0/oct/x86_64-unknown-linux-gnu/PKG_ADD
/usr/local/share/octave/4.0.0/m/deprecated/PKG_ADD
/usr/local/share/octave/4.0.0/m/image/PKG_ADD
/usr/local/share/octave/4.0.0/m/optimization/PKG_ADD



Olaf Till on comment #3

> I think disallowing PKG_ at package root is not the right solution.
>
> Suggestion: PKG_ at package root always only into one directory, either arch-dependent or -independent. PKG_ directives from source-files or installed files also each directive only in one of arch-dep. or -indep. directory. Doesn't actually matter into which. And then: All PKG_ADD directives of a package should be called only after having added both arch-dep. and -indep. directory to the path, and correspondingly for PKG_DEL.


I don't understand why disallowing PKG_ at the root of the package is bad. I can't see any case where requiring it to be on a specific directory is worse. You suggest to support this by making sure to copy the files only to one of the directories. But we can drop this whole ambiguity by require the package developer to place the file in the directory that makes the most sense.


== Julien Bect suggestion ==

I'm not familiar with the binary distributions side of it, but the proposal for the source releases is in line with my idea.

> I just realized that when you said "I think the best fix is to no longer accept PKG_ADD and PKG_DEL files at the root of the package" (see #1), perhaps did you actually mean "not at the root of the package, ./inst and ./src instead" ? In which case this is very close to my proposition.


Yes. This is exactly what I proposed. Point is, I can't see a user-case where it makes sense to have the same PKG_ADD and PKG_DEL scripts in both arch-dependent and independent directories.  This means that the only reason to support the files at the root of the package, is when there is only one of such directories. But if that's the case, why not require the files to be place in the correct directory? If package developers place the PKG_ADD on the directory that is meant to be execute from, none of this problems would happen.

> 1) No longer accept PKG_ADD/PKG_DEL files at the root of the package tarball.


Agreed.

> 2) Accept PKG_ADD/PKG_DEL files in ./inst, or in any subdirectory of ./inst. Those files are copied at the corresponding location in the arch-independent directory.


Not sure if I understood all the implications of it correctly, but I kind of agree.  Agree with accepting files in ./inst and its subdirectories (seems like pkg() removes non .m files, that should not happen).  Do not agree on having PKG_ADD on subdirectories executed when loading the package. It should be the PKG_ADD at the root of inst/ that is responsible for calling the others (most likely by adding the subdirectories to the path).

> 3) Also, accept PKG_ADD/PKG_DEL files in ./src (for source packages) or in ./inst/ARCH-NAME (for binary packages). Those files are copied in the arch-dependent directory.


I'm not that familiar with this binary distributions but sounds reasonable.

> 4) Keep accepting PKG_ADD/PKG_DEL directives as well.


Yes please.

> 5) When building a binary package, copy PKG_ADD/PKG_DEL directives into PKG_ADD/PKG_DEL files (this is what we already do) but leave the resulting files where they are (in particular, do not move them at the root of the tarball)
>
> 6) When installing a binary package, do not copy PKG_ADD/PKG_DEL directives into PKG_ADD/PKG_DEL files a second time (once is enough !)


This requires pkg() knowing if it's installing a binary package or not and adds extra logic dependent on the type of installation. Ideally, we would keep differences to the minimum. Maybe the opposite is better, to not have binary distributions handling PKG_ADD at all, and leave that job for the actual pkg install. Problem is, a binary install no longer has the source. There is another issue related it. demos and tests from functions in src/ are lost during install because the source files are removed after building. If they are kept around for this purpose, they would still be available in binary distributions. This would also make them available for the binary installs.

== Removal of PKG_ and PKG_DEL directives by Philip ==

This is not going to happen. Even core makes use of it. On the io and stk package, you may be using PKG_ADD as a script that gets run when loading a package. But that's not its main use and original purpose.  The original idea was code that runs when a specific function is added to the path.  Because of this, it makes a lot of sense to have it on the respective file and not on a general PKG_ADD file.

> PKG_ADD / PKG_DEL are easily tracked. What if some function containing PKG_ADD/PKG_DEL is removed from a package, or if functions accidentally containing PKG_ commented code are moved between packages?


In that case use a PKG_ADD script. pkg() will append any PKG_ADD directives to that script. But code that is related to a specific function should go into that function. Your argument ignores the opposite side (removing a function from a package and forgetting to remove its code from PKG_ADD script).

> * It is simpler and more clean. I do not like to have special code in a function file that needs to be there for purposes the function itself isn't meant for. (Carnë suggested to have PKG_ADD/PKG_DEL in one particular .m-file in the io package)


I guess that's a opinion.  On my view, the io package is about IO of different formats, not about xls support. In that view, it makes sense to check for xls only when such function gets added to the path.  But I can see the other side too. What I don't see is how it's preferable to have a workaround that removes PKG_ADD from the oct-files directory (mostly because I don't trust pkg() and post_install() enough to do it right in all situations. The risk of messing up and removing user files is too large). And what will you do once you actually need a PKG_ADD on the oct files directory?

Carnë Draug <carandraug>
Group Member
Tue 30 Jun 2015 11:45:11 AM UTC, comment #14: 

@Carnë: what do you think about the approach in comment #8 ?

I just realized that when you said "I think the best fix is to no longer accept PKG_ADD and PKG_DEL files at the root of the package" (see #1), perhaps did you actually mean "not at the root of the package, ./inst and ./src instead" ? In which case this is very close to my proposition.

Julien Bect <jbect>
Tue 30 Jun 2015 11:37:23 AM UTC, comment #13: 

Hello everyone,

What I said earlier about PKG_ADD/PKG_DEL directives is not 100% accurate. They are not a feature of addpath/rmpath.

The PKG_ADD/PKG_DEL directives in Octave core are parsed at make-time by the following script:

http://hg.savannah.gnu.org/hgweb/octave/file/f5d9e57d4380/scripts/mk-pkg-add

Julien Bect <jbect>
Wed 24 Jun 2015 05:59:59 AM UTC, comment #12: 


> The other thing is that we have some legacy of
> existing packages. How are we going to cope with them?


In comment #8, I think it is easy to keep a "legacy mode" for some time: if packages have PKG_ADD/PKG_DEL files at the root of the package tarball (but not in ./src) they should be copied in the arch-dependent directory, as is currently done.

Julien Bect <jbect>
Wed 24 Jun 2015 05:57:03 AM UTC, comment #11: 

One more point about PKG_ADD/PKG_DEL directives: they are not specific to Octave Forge packages. This is a generic feature of addpath/rmpath, which is even used in some places in Octave core:

http://hg.savannah.gnu.org/hgweb/octave/file/f5d9e57d4380/scripts/image/autumn.m#l32

http://hg.savannah.gnu.org/hgweb/octave/file/f5d9e57d4380/scripts/optimization/fminbnd.m#l61

They are also rather natural to use for autoload statements:

http://hg.code.sf.net/p/octave/image/file/b348244c0153/src/conndef.cc#l190

(Thanks Carnë for showing me all this)

So, my opinion is that we have to design a clean solution that takes into account the existence of PKG_ADD/PKG_DEL directives (not ignore them or propose to remove them).

I believe that comment #8, points 4--6, provide such a solution.

> What advantage do such directives have over
> what Carnë (below in pt. 2 & 3) and I suggest?
> If there's a use case for directives with
> clear advantages over Carnë's/my suggestion


I don't think Carnë ever suggested to get rid of PKG_ADD/PKG_DEL directives.

Julien Bect <jbect>
Wed 24 Jun 2015 05:47:11 AM UTC, comment #10: 

Philip,

> @Julien, comment #7:
> I never heard of PKG_ADD / PKG_DEL directives,
> I haven't found documentation for it either - but
> I had no reason to search.


Yes, PKG_ADD/PKG_DEL directives are properly documented:

https://www.gnu.org/software/octave/doc/interpreter/PKG_005fADD-and-PKG_005fDEL-Directives.html

This is what Carnë suggested to you, in the case of the io package, to avoid moving PKG_ADD/PKG_DEL files during post_install. But for sure, there are other ways to achieve the same result (for instance, points 1--3 in my comment #8).

Julien Bect <jbect>
Tue 23 Jun 2015 09:24:41 PM UTC, comment #9: 

@Julien, comment #7:
I never heard of PKG_ADD / PKG_DEL directives, I haven't found documentation for it either - but I had no reason to search. So there's my motive.

What advantage do such directives have over what Carnë (below in pt. 2 & 3) and I suggest?
If there's a use case for directives with clear advantages over Carnë's/my suggestion, well let's keep them. If not I'd opt for the simplest solution, for the reasons I already described.

The other thing is that we have some legacy of existing packages. How are we going to cope with them?


@Carnë, comment #8 item 8:
binary modules and m-files get lumped together when installing packages in Linux HOME dirs, e.g., for io I get:

~/octave/io-2.2.8: all m-files
~/octave/io-2.2.8/x86_64-unknown-linux-gnu-api-v50+: all .oct files


so the binary modules reside in a subdir of the arch-independent files.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 23 Jun 2015 04:15:30 PM UTC, comment #8: 

I propose something else.

1) No longer accept PKG_ADD/PKG_DEL files at the root of the package tarball.

2) Accept PKG_ADD/PKG_DEL files in ./inst, or in any subdirectory of ./inst. Those files are copied at the corresponding location in the arch-independent directory.

3) Also, accept PKG_ADD/PKG_DEL files in ./src (for source packages) or in ./inst/ARCH-NAME (for binary packages). Those files are copied in the arch-dependent directory.

4) Keep accepting PKG_ADD/PKG_DEL directives as well.

5) When building a binary package, copy PKG_ADD/PKG_DEL directives into PKG_ADD/PKG_DEL files (this is what we already do) but leave the resulting files where they are (in particular, do not move them at the root of the tarball)

6) When installing a binary package, do not copy PKG_ADD/PKG_DEL directives into PKG_ADD/PKG_DEL files a second time (once is enough !)

7) To make things perfectly clear for package developpers, specify in which order the two directories (on platforms where they are separate) are loaded and unloaded, and therefore in which order the corresponding PKG_ADD/PKG_DEL files are executed.

8) On platforms where the two directories are not separated (does this exist ?), merge the PKG_ADD/PKG_DEL files, when both exist, in the same order as specified in 7).

How does that sound ?

Julien Bect <jbect>
Tue 23 Jun 2015 03:59:48 PM UTC, comment #7: 

Hello Philip,

If I understand correctly, you suggest to get rid of PKG_ADD/PKG_DEL directives completely (just to be clear, by "directive" I mean the ones that are included as comments).

This is not the same as what Olaf suggested (comment #3). Olaf didn't suggest, I think, to get rid of PKG_ADD/PKG_DEL directives.

Julien Bect <jbect>
Tue 23 Jun 2015 03:46:38 PM UTC, comment #6: 

I like Olaf's suggestion the most:

Simply let package maintainers/developers put PKG_ADD / PKG_DEL in the most suitable directory (arch-independent or-dependent) for a certain package, and let pkg.m just copy it over.

  • Easy to grasp for developers;
  • No need for maintenance of maybe fragile code for parsing and extracting of stuff from comments;
  • PKG_ADD / PKG_DEL are easily tracked. What if some function containing PKG_ADD/PKG_DEL is removed from a package, or if functions accidentally containing PKG_* commented code are moved between packages?
  • It is simpler and more clean. I do not like to have special code in a function file that needs to be there for purposes the function itself isn't meant for. (Carnë suggested to have PKG_ADD/PKG_DEL in one particular .m-file in the io package)


Even better (as Olaf also mentions) would be if pkg.m could be adapted such that PKG_ADD & PKG_DEL are only invoked after the entire package is loaded.
Is there a certain and consistent order in which the arch-dependent and arch-independent directories are loaded? (E.g., for the io package it is always the arch-independent one that is loaded last.)
If so I'd suppose pkg.m could simply be patched to put PKG_* in the directory that is loaded lastly rather than always in the arch-dependent one.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 23 Jun 2015 12:06:15 PM UTC, comment #5: 


> I think disallowing PKG_ at package root is not the right solution.
>
> Suggestion: PKG_ at package root always only into one directory, either arch-dependent or -independent. PKG_ directives from source-files or installed files also each directive only in one of arch-dep. or -indep. directory. Doesn't actually matter into which. And then: All PKG_ADD directives of a package should be called only after having added both arch-dep. and -indep. directory to the path, and correspondingly for PKG_DEL.


If the explanation in my previous post is ok, then this won't really solve the problem.

What needs to be done, I think, is to prevent 'pkg build' from writing the PKG_ADD/PKG_DEL directives in the PKG_ADD/PKD_DEL files that are bundled into the binary package tarball. Otherwise, these directives will end up being duplicated (in two separate files, as it is currently the case, or in one single file if we insist on having PKG_ADD/PKG_DEL files in only one place, but this is not better).

Another solution would be to mark binary package tarballs as a special kind of package tarball, for which PKG_ADD/PKG_DEL do not need to be processed during 'pkg install' (since they have been processed during 'pkg build'). In other words, pkg should process PKG_ADD/PKG_DEL directives only when building a binary tarball, or when building+installing from source, but not when installing from a binary tarball.


Julien Bect <jbect>
Tue 23 Jun 2015 11:53:06 AM UTC, comment #4: 


> I wasn't aware of pkg build copying PKG_ at package root to both arch-dependent and -independent directories. But this should surely not be done.


Again, I don't think it does.

'pkg build' puts the PKG_ADD/PKG_DEL files at the root of the package tarball, which to me is neither the root of the arch-independent directory (./inst) nor the root of the arch-dependent directory (e.g., ./inst/i686-pc-linux-gnu-api-v50+).

It is later, when 'pkg install' is called on the binary package tarball, that the duplication problem occurs: the existing PKG_ADD/PKG_DEL are installed into the arch-dependent directory, and new ones are created from the directives and installed into the arch-independent directory.

Julien Bect <jbect>
Tue 23 Jun 2015 08:37:32 AM UTC, comment #3: 

I wasn't aware of pkg build copying PKG_ at package root to both arch-dependent and -independent directories. But this should surely not be done.

I think disallowing PKG_ at package root is not the right solution.

Suggestion: PKG_ at package root always only into one directory, either arch-dependent or -independent. PKG_ directives from source-files or installed files also each directive only in one of arch-dep. or -indep. directory. Doesn't actually matter into which. And then: All PKG_ADD directives of a package should be called only after having added both arch-dep. and -indep. directory to the path, and correspondingly for PKG_DEL.

The latter (after 'And then:') was implemented in the unapplied patch:

https://savannah.gnu.org/patch/?7976

Olaf Till <i7tiol>
Group Member
Tue 23 Jun 2015 06:28:22 AM UTC, comment #2: 


> PKG_ADD and PKG_DEL files can be defined in different ways. At the root of the package, and as comments in the source files. In the first case, the files are copied both to the architecture dependent and independent directories. In the later, a PKG file is created for each of the directories.


Hello Carnë,

My understanding of what pkg really does is slightly different...

In the case of directives, I think that only one set of PKG_ADD/PKG_DEL files is created by 'pkg build'. But then, when 'pkg install' is called, the files created by 'pkg build' are moved to the arch-dependent directory, and a new set of files is created from the directives in the arch-independent directory. Am I wrong ?

In the case of PKG_ADD/PKG_DEL files located at the root of the source tarball, I have no idea what happens when calling 'pkg build' and then 'pkg install'. But is there any such package in Octave Forge currently ?

@++
Julien

PS: In my opinion, the root of this problem is a lack documentation, and perhaps also the lack of a formal specification, of what pkg is supposed to do (which operations are carried out in which order, what is the expected directory structure, what is allowed in pre_install / post_install files, etc.).

Julien Bect <jbect>
Sun 21 Jun 2015 12:16:39 PM UTC, comment #1: 

I have attached a simple package to exemplify the issue:


octave-gui:1> pkg install "test-bug45369.tar.gz"
foo() was called
octave-gui:2> pkg build binary "test-bug45369.tar.gz"
foo() was called
octave-gui:3> pkg install "binary/test-bug45369-1.0.0.tar.gz"
foo() was called
'foo' undefined near line 2 column 1
error: called from '/home/carandraug/octave/test-bug45369-1.0.0/x86_64-unknown-linux-gnu-api-v50+/PKG_ADD' in file /home/carandraug/octave/test-bug45369-1.0.0/x86_64-unknown-linux-gnu-api-v50+/PKG_ADD near line 2, column 1


Inspection of the two tarballs will show:

1. no PKG_ADD files in the original source release.

2. a PKG_ADD directirve as comment in inst/foo.m

3. a PKG_ADD file at the root of the binary distribution (this file should not have been created)

4. the error message shows that installation of the binary package, places the PKG_ADD in the arch dependent directory.  Error is caused because foo() will only be added to the path later.

(file #34284)

Carnë Draug <carandraug>
Group Member
Sun 21 Jun 2015 11:55:19 AM UTC, original submission:  

PKG_ADD and PKG_DEL files can be defined in different ways. At the root of the package, and as comments in the source files.  In the first case, the files are copied both to the architecture dependent and independent directories.  In the later, a PKG file is created for each of the directories.

pkg() has an option "build" to generate binary distribution of packages. When such distribution is created, pkg() parses the source files for PKG_ADD and PKG_DEl directives.  It then places them at the root of the package. This means that installation of such binary distribution will copy the PKG files to both directories.

This has 2 negative side effects:

  • the PKG files will be executes twice (probably not what the developer wanted);


  • executing the PKG files may fail if they call functions that belong to the package (which is often the case)


On the case of the stk package (where this bug was reported), PKG_ADD calls stk_init().  This is a m function and is defined as a comment on a m file.  Installing this package means that a single PKG_ADD file is created on the architecture independent directory. Everything works fine.  However, when using "pkg build", the PKG_ADD file is copied to the root of the package and then installed in both directories. The architecture dependent is added to the path first, when stk_init() is not defined yet causing an error when loading the package.

See #45362 for details and a test case.

I think the best fix is to no longer accept PKG_ADD and PKG_DEL files at the root of the package.  If they are meant to be executed when adding the package to the package, chances are the maintainer only wants it executed once.  The only use case I can think for it is when a package has only arch dependent or independent functions in which case they could have this files there too.

Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36730:  octave-4.0.1-1.pkg.patch added by matzeri (16KiB - text/x-diff - pkg build version for 4.0.1)
file #36706:  pkg_build_bug_45369.cset added by carandraug (18KiB - application/octet-stream)
file #34284:  test-bug45369.tar.gz added by carandraug (644B - application/gzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by matzeri (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by i7tiol (Posted a comment)
  • -email is unavailable- added by jbect (Posted a comment)
  • -email is unavailable- added by matzeri
  • -email is unavailable- added by carandraug (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 group members can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-16 mtmiller Open/ClosedOpen Closed
    2016-05-16 carandraug StatusNone Fixed
    2016-03-22 matzeri Attached File- Added octave-4.0.1-1.pkg.patch, #36730
    2016-03-21 carandraug Attached File- Added pkg_build_bug_45369.cset, #36706
    2015-06-21 matzeri Carbon-Copy- Added matzeri
    2015-06-21 carandraug Attached File- Added test-bug45369.tar.gz, #34284

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code