search

gap-packages / io

GAP package IO to do input and output
https://gap-packages.github.io/io/
Other
14 stars 14 forks source link

take into account CPPFLAGS #103

Closed jgmbenoit closed 2 years ago

jgmbenoit commented 2 years ago

Description: upstream fix: CPPFLAGS support Render Makefile.gappkg to take into account CPPFLAGS. Origin: vendor, Debian Author: Jerome Benoit calculus@rezozer.net Last-Update: 2021-10-10

codecov[bot] commented 2 years ago

Codecov Report

Merging #103 (453cf94) into master (0ac79a7) will decrease coverage by 2.67%. The diff coverage is n/a.

:exclamation: Current head 453cf94 differs from pull request most recent head e72372d. Consider uploading reports for the commit e72372d to get more accurate results

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   61.56%   58.88%   -2.68%     
==========================================
  Files          15       15              
  Lines        4946     4653     -293     
==========================================
- Hits         3045     2740     -305     
- Misses       1901     1913      +12
Impacted Files Coverage Δ
gap/realrandom.gi 31.57% <0.00%> (-9.05%) :arrow_down:
gap/iohub.gi 11.85% <0.00%> (-5.23%) :arrow_down:
src/io.c 65.75% <0.00%> (-4.00%) :arrow_down:
gap/background.gi 36.51% <0.00%> (-3.80%) :arrow_down:
gap/pickle.gi 69.08% <0.00%> (-1.44%) :arrow_down:
gap/io.gi 59.38% <0.00%> (-1.42%) :arrow_down:
gap/callwithtimeout.gi 52.00% <0.00%> (-0.95%) :arrow_down:
gap/http.gi 62.06% <0.00%> (-0.44%) :arrow_down:
fingolfin commented 2 years ago

This file is copied from the central "official" version at https://github.com/gap-system/gap/blob/master/etc/Makefile.gappkg, so patching it here really isn't doing any good long term.

Could you maybe also expand on why this is a beneficial / necessary change? It seems that instead you could just add the CPPFLAGS value to CFLAGS (or in the case of a package using C++, to CXXFLAGS, but that's not the case here)

jgmbenoit commented 2 years ago

I looked for the original version but I could not find it neither in the header of the file nor elsewhere.

fingolfin commented 2 years ago

The salient point really is: what is the motivation for this patch? What problem does it fix?

jgmbenoit commented 2 years ago

The Debian Helper machinery use CPPFLAGS to pass options to the compilers in a transparent way. In particular the "hardening-wrapper" machinery uses it. There is even a (popular) lintian tag for it: hardening-no-bindnow. So I guess it is an expected usage in UNI*X. I also note that this Makefile fragment is an helper for installing/packaging GaP from the source. I could add tricky codes in my debian/rules file (or elsewhere) to pass the options, but I think it more beneficial to put the expected usage in the fragment itself, hence the patch.

fingolfin commented 2 years ago

Made the general patch in the master copy in https://github.com/gap-system/gap/commit/d9a95fa0f3ab61abdf8da92f3907ad05919c8070 some months ago (and then forgot about it ...) and will now look into successively upgrading this in packages that use it.

fingolfin commented 2 years ago

Thank you for your submission. A different fix but with hopefully the same effect should be in the next IO release