Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libmatroska: fix REQUIRES_devel, drop libtool file, add TEST(). #671

Merged
merged 1 commit into from Jul 6, 2016

Conversation

@fbrosson
Copy link
Member

fbrosson commented Jun 29, 2016

  • libmatroska.la has a path which will become invalid if libebml gets updated. Dropping that .la file is OK because we don't need it.
  • Add devel:libebml to REQUIRES_devel.
  • Also add gcc to REQUIRES_devel on effective architectures other than x86_gcc2 to make sure it will have libstdc++.
  • Add TEST() with make check.
@korli
Copy link
Contributor

korli commented Jun 29, 2016

Requiring gcc isn't what it should be. You'd like something like devel:libstdc++?

@fbrosson
Copy link
Member Author

fbrosson commented Jun 29, 2016

Well, I had tried using devel:libstdc++ in REQUIRES_devel, but it does not work because gcc does not have it in its PROVIDES, althought devel:libstdc++ is really in gcc. BTW:

$ pkgman search lib:libstdc++
Status  Name         Description                                                     
-------------------------------------------------------------------------------------
S       gcc_syslibs  C/C++-runtime shared libraries, needed to execute C/C++ programs

$ pkgman search devel:libstdc++
No matching packages found.

Should we add a devel:libstdc++ to the PROVIDES of each gcc?

@waddlesplash
Copy link
Member

waddlesplash commented Jun 29, 2016

I'm confused. Why is such a dependency needed? Isn't gcc_syslibs_devel needed to compile anything, and thus anything that wanted matroska would also have it?

@fbrosson
Copy link
Member Author

fbrosson commented Jun 29, 2016

As we know, some packages have .la files that include paths to the libstdc++.la that are provided by gcc, but these start with /packages/gcc and include the revision of the gcc package that is being used. So if we want these files to remain valid even if the gcc package is upgraded with the same version but a bumped revision, then we need to fix that path to make it use the package-links directory. And these package-links are only available if we add gcc to REQUIRES_devel.

@pulkomandy
Copy link
Member

pulkomandy commented Jun 30, 2016

The current policy has been to simply remove the .la files from packages. They are designed for use with libtool, but in our case libtool works just fine without them. No need to clutter the development dirs with these files, then.

That being said, it would still be nice if the gcc package properly exposed all included libs. This would make it possible to reorganize the packages later on (split it further, merge it with something else, or simply rename it) without breaking everything as would be the case with packages which depend directly on the package name or rely on it being installed by the system.

@fbrosson
Copy link
Member Author

fbrosson commented Jun 30, 2016

The current policy has been to simply remove the .la files from packages.

I have recently came accross one package, midnight commander, that could not be built because it could not find libintl.la. So the policy is not ok for all packages. Moreover, when I tried to build handbrake it failed because libmatroska was pre-installed but had a path to a libstdc++.la that came from an old version of gcc which was no longer available. So I simply could not use the pre-installed version of libmatroska. I had to use haikuporter -E handbrake to enter the chroot and use grep in there to identify which dependency of handbrake was pulling the invalid path. If libmatroska had had its .la file fixed, it would not have happened.
Finally, as we all know, the .la files are really tiny files. IMHO keeping them and making sure they remain valid is not going to clutter the development dirs at all. I think we should only drop the static libs, but this will be a long task because we first need to make sure no other package needs them.
My conclusion is that it is much better to just fix the .la files when they are found to be wrong. Removing them to find out later that some other packages need them is not going to help us.

That being said, it would still be nice if the gcc package properly exposed all included libs.

I agree, but I don't feel comfortable with changing gcc without @korli's approval.

All this brings me to this qustion: do I have green lights to merge this PR as is, i.e. keeping and fixing the libmatroska.la file? Or should I close this PR without merging? Thank you.

EDIT: I forgot to say I would be OK with making this recipe depend on devel:libstdc++ instead of gcc if we first change gcc to expose devel:libstdc++ in its PROVIDES.

@mmuman
Copy link
Member

mmuman commented Jun 30, 2016

FWIW I wrote a fixLibtoolArchives helper although I'm not sure it is fully correct.

@fbrosson
Copy link
Member Author

fbrosson commented Jun 30, 2016

Cool, thanks! I'll try it.

@korli
Copy link
Contributor

korli commented Jul 3, 2016

Please try to drop the libtool file before fixing it. This avoids several headaches down the road.

@fbrosson
Copy link
Member Author

fbrosson commented Jul 4, 2016

OK. The only recipes which depend on libmatroska are those of VLC, so I'm quite sure it will be OK to drop both the static lib and the libtool file. I'll test that. If it turns out that we can drop them, should I drop both the static lib and the libtool file or should I drop the static lib only?

@fbrosson fbrosson changed the title libmatroska: fix REQUIRES_devel and paths in libmatroska.la. libmatroska: fix REQUIRES_devel, drop libtool file, add TEST(). Jul 4, 2016
@fbrosson
Copy link
Member Author

fbrosson commented Jul 4, 2016

I'm dropping libmatroska.la and keeping libmatroska.a.

BTW, I forgot to tell that I tried using fixLibtoolArchives in the recipe but it did not work. It uses /packages/$portRevisionedName/lib~ and only works for dependencies declared as lib:lib*. So recipes which would have devel:lib* in their REQUIRES_devel but no corresponding lib:lib* in their REQUIRES would not be handled. I think fixLibtoolArchives should first try using /packages/$portRevisionedName/devel~ and, if that directory is missing, try /packages/$portRevisionedName/lib~.
Well, since we don't want to keep .la files anymore we don't need fixLibtoolArchives.

  • Also add gcc to REQUIRES_devel on effective architectures other than x86_gcc2 to make sure it will have libstdc++.

I'm not sure we need this. Shall I drop the dependency on gcc?

* libmatroska.la has a path which will become invalid if libebml gets
  updated. Dropping that .la file is OK because we don't need it.
* Add devel:libebml to REQUIRES_devel.
* Add TEST() with "make check".
@waddlesplash waddlesplash merged commit e343742 into haikuports:master Jul 6, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fbrosson fbrosson deleted the fbrosson:libmatroska branch Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.