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

Linux packaging #1

Merged
merged 5 commits into from
Apr 6, 2013
Merged

Linux packaging #1

merged 5 commits into from
Apr 6, 2013

Conversation

reddwarf69
Copy link

The idea is being able to change

make libdir=%{buildroot}%{_libdir} includedir=%{buildroot}%{_includedir}/ebml install_sharedlib install_headers

which requires the spec file knowing the names of the specific rules and the headers installation path, for

%make_install prefix=%{_prefix} libdir=%{_libdir} NO_STATIC=yes

There is no need for -fno-gnu-keywords (which was implied by -ansi) in the
current code.
-ansi disables GCC built-in functions, which add optimizations and extra
security checks in some cases. It may be good to use it (and -pedantic) while
developing to try to keep the code ANSI standard, but users should have binaries
compiled without the option.
@mbunkus
Copy link
Contributor

mbunkus commented Mar 28, 2013

Thanks for the work. A couple of objections:

  1. I've added the warning flags for a reason. If you don't like them then override the corresponding variable from outside the Makefile, but don't remove them unconditionally.
  2. Using Linux-style EOL on Makefile is more or less fine, but that's just a fix for a single file because it suited your purposes. I'd prefer a solution that fixes all the files except the ones that are solely for Windows programs. Those exceptions are in make/vc6 and make/vc7.
  3. The DESTDIR part is fine. However, I don't really get the need for NO_STATIC. What is the big difference between make install_sharedlib install_headers and make NO_STATIC=1 install? Meaning... I don't see the big advantage. Granted, there's no big disadvantage here either, but it doesn't actually add anything new -- there are now two ways to achieve the very same thing. More code to maintain etc.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 28, 2013

And:

  1. Will you offer a similar pull request for libmatroska after we've polished this one?

@reddwarf69
Copy link
Author

  1. May I know the reason? Until recently I didn't care, but somebody made me notice the problem with the gcc built-in functions when using -ansi. Now to the best of my knowledge there is no good reason to use -ansi in release builds and unless somebody makes me notice my error I will submit similar patches to other projects.
  2. So you prefer Linux style EOL by default? I don't really care, I just changed it because I guess that file will be modified mostly by Linux users and they could use an editor that mixes different EOLs. I did put it as the last commit specifically in case you would not want it.
  3. The problem with "install_sharedlib install_headers" for me is that I don't know if you will ever change the name of those rules or add extra ones (install_pkgconfig?). Being a standard de-facto I trust the "install" rule as something that will be always there and will install everything needed.
  4. Sure, whatever changes go to libebml and will mimic them in libmatroska.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 28, 2013

  1. Hmm, I thought it was solely about being as strict about following standards. I don't know what you refer to with "the problem with gcc built-in-functions". If there's a real issue then I'm definitely not against removing -ansi again, but I'd like to read some more about this before deciding.
  2. Definitely, yes. The repos are currently pretty mixed, unfortunately, mostly due to Steve Lhomme and me using different OSes -- and due to me importing the old Subversion repos 1:1. I haven't cleaned up such things, but if it will be done then it should be done consistently.
  3. I see your point. I'd rather the options to be named slightly differently, though. What about make link=shared install? That's how it's named in Boost, mostly. link could have three values: both (which would also be the default), shared and static. Please also adjust the rules for the all: target, not just for the install: target.
  4. Great.

@reddwarf69
Copy link
Author

  1. I explained it a bit in the commit message ("-ansi disables GCC built-in functions, which add optimizations and extra security checks in some cases"). An example of lost optimization opportunity is shown in http://gcc.gnu.org/gcc-4.7/changes.html ("for hosted compilations where stpcpy is available"). Since stpcpy is not available when using "-ansi" that example doesn't apply. The security cases come from the built-ins from http://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html, but I don't have a good specific example right now.
  2. OK. I will look into it.
  3. OK

Once the -ansi things is decided (I don't really have any problem keeping -fno-gnu-keywords) I will try to find some time and redo all this.

@mbunkus
Copy link
Contributor

mbunkus commented Mar 29, 2013

Dang. I remembered wrong; I didn't add -ansi (nor -fno-gnu-keywords). I added some of the other warning flags, though. So yes, please remove -ansi, also remove -fno-gnu-keywords while you're at it (I've read up on what that option does; it's nothing we need anyway).

@reddwarf69
Copy link
Author

This should be it.

There is another problem. The FSF address in the LICENSE.LGPL and the sources is outdated. No idea about the exact legal implications but openSUSE build tools complain about that.
I was going to fix it. But then I saw the reference to http://www.matroska.org/license/lgpl/ is also broken. So I leave it to you to handle as you prefer.

@mbunkus
Copy link
Contributor

mbunkus commented Apr 6, 2013

Very nice, thanks, I'll merge it.

As for the license: The address (the full header) in LICENSE.LGPL should be fixed to match the text in http://www.gnu.org/licenses/lgpl-2.1.html, of course. All source files still mentioning the license on matroska.org should have their URLs changed to http://www.gnu.org/licenses/lgpl-2.1.html as well. If you'd be willing to do that and provide another pull request then that would be very, very nice (I'm pretty busy for the next two weeks).

mbunkus added a commit that referenced this pull request Apr 6, 2013
@mbunkus mbunkus merged commit b712924 into Matroska-Org:master Apr 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants