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

Fix gac to work with /bin/sh that is not bash #1292

Merged
merged 3 commits into from
May 4, 2017

Conversation

fingolfin
Copy link
Member

No description provided.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Apr 25, 2017
@fingolfin fingolfin requested a review from frankluebeck April 25, 2017 17:16
etc/ci.sh Outdated
@@ -79,6 +79,12 @@ cd profiling
./configure $CONFIGFLAGS --with-gaproot=$BUILDDIR
make V=1

# Compile browse package to test gac
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a cd .. here

@codecov
Copy link

codecov bot commented Apr 25, 2017

Codecov Report

Merging #1292 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1292   +/-   ##
=======================================
  Coverage   61.36%   61.36%           
=======================================
  Files         916      916           
  Lines      277218   277218           
  Branches    15335    15335           
=======================================
  Hits       170118   170118           
  Misses     103181   103181           
  Partials     3919     3919

@fingolfin
Copy link
Member Author

Actually, trying to test build Browse like that won't work, because it also needs a source code change. So I switched this to edim now.

@frankluebeck
Copy link
Member

Without this PR it was not possible to gac-compile the kernel modules of the EDIM and Browse packages on a Debian 8 or on an Ubuntu 16.04 system (which start /bin/sh scripts in strict sh-compatibility mode). With this PR it works again, thanks.

I noticed that for example for the EDIM package after compilation (on Debian 8, x86_64) the bin
directory contains 76 kB. With GAP 4.8.6 it contains only 16 kB. It seems that in the new setup one can delete everything except the generated *.so file (26 kB, which in turn can be striped to 8 kB). Shouldn't the additional .libs directory and the *.la file be cleaned up by gac?

@fingolfin
Copy link
Member Author

Yes, perhaps these should be cleared (although we might have to reconsider that when implementing make install -- but that's of course not an argument about cleaning these files for now, I just mention it for completeness).

However, this PR is strictly meant to fix the bug; I'd prefer to not also add other changes in here, but instead get it merged ASAP. So perhaps this could be filed as a feature request issue? Or feel free to submit a PR implementing it.

To this end, I just pushed a new revision, in an attempt to make the CI tests pass (the previous iteration failed because gac does not currently work on HPC-GAP, see #1295. The short term workaround is to just not test it on HPC-GAP).

@fingolfin
Copy link
Member Author

The failure on Windows is another gac bug; I think I know how to fix it, will look into it when I have some time.

@fingolfin
Copy link
Member Author

On Windows, we need to link the module (e.g. edim.so resp. edim.dll) against gap.dll (resp. libgap.la). The main "problem" doing that is to teach gac that. So we'll either need to let gac know when we are on Windows (by tweaking how we generate it), or else pass the required linker options directly to it from configure. Either works, it's a matter of taste in the end, I guess.

But I don't have time right now to work on it.

However, we skip the edim test on HPC-GAP and cygwin, as gac is
currently broken on both systems.
@fingolfin
Copy link
Member Author

The new revision of this PR disables the edim test on Windows, too; and I created issue #1298 to log the problem.

@fingolfin
Copy link
Member Author

From my POV this could be merged now.

@ChrisJefferson ChrisJefferson merged commit 7cda48b into gap-system:master May 4, 2017
@fingolfin fingolfin deleted the mh/fix-gac branch May 5, 2017 13:19
@olexandr-konovalov olexandr-konovalov added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants