Skip to content

Conversation

@fingolfin
Copy link
Member

@fingolfin fingolfin commented Apr 15, 2016

This PR for the stable-4.8 branch

The latter script is meant to replace InstPackages.sh.

Before this merged, a few things need to happen (probably by adding commits to this PR):

  • improve BuildPackages.sh a bit more (see issue BuildPackages.sh vs. InstPackages.sh #737)
  • somebody needs to verify that the removal of the special case for anupq really did not break anything (it didn't for me, but then I don't know why it was added in the first place)

This way, it is much more please to read as a text file, i.e. without
rendering it on a website.
While many markdown parsers will automatically recognize HTTP urls
and emails, not all do that. And if an URL is followed by e.g. a
period, when rendering the markdown to HTML, that period may
inadvertently end up as part of the hyperlink.
The term UNIX is trademarked and refers to *the* UNIX operating system,
but not its derivates.

Mac OS X actually is a direct descendant of a version UNIX, and thus
directly related to UNIX (it is derived from FreeBSD, which in turn is
derived from BSD UNIX). Thus it seems weird to refer to "Unix and OS X".
However, some OS X users may not realize that a section about "Unix"
is relevant for them, so I sometimes used expressions like "Unix
(including OS X)".

By the way, note that Linux (which most people will consider a Unix)
strictly speaking is NOT a direct member of the Unix family: While
developed to be compatible with UNIX, it (at least originally) shares
no code with it, and thus is a "UNIX clone". So in a sense, it is a
less "a Unix" than OS X. Of course that just shows how silly that
distinction is, nothing else. I just mention it to reinforce that if
we call Linux a Unix, then we should do so the same for OS X.
- Don't promise a binary distribution for OS X "soon": It helps nobody
  and just makes us look foolish when we don't deliver. And *if* we
  ever offer one, we can still tell people about it.
- Fix spelling of 'Xcode', and update the installation instruction to
  match what was the standard for the past couple years. People on older
  systems will (still!) have to ask us or google to get instructions,
  but at least this way we don't needlessly confuse people who use
  OS X 10.9 or later.
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: documentation Issues and PRs related to documentation topic: build system labels Apr 15, 2016
@fingolfin fingolfin added this to the GAP 4.8.4 milestone Apr 15, 2016
@ChrisJefferson
Copy link
Contributor

ChrisJefferson commented Apr 15, 2016

The anupq comes from the InstPackages32.sh script, which contains:

cd anupq*
./configure CFLAGS=-m32 LDFLAGS=-m32
make CFLAGS=-m32 LOPTS=-m32
cd ..

So at some point, someone thought these needed specially passing, and I just copied that on down. If a 32-bit build on a 64-bit machine just works, then I don't think there has to be a special case anymore.

INSTALL.md Outdated

or

./configure ABI=32
Copy link
Contributor

@ChrisJefferson ChrisJefferson Apr 15, 2016

Choose a reason for hiding this comment

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

These are identical?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy & paste error, thanks for spotting, will fix (but I'll give people some time to review more first :)

@ChrisJefferson
Copy link
Contributor

My patch to BuildPackages.sh was just the following. Feel free to modify or ignore as you wish. I think we can go further than this after your commit (perhaps gathering the packages that build, and don't build, to print some simple statistics at the very end).

+++ b/bin/BuildPackages.sh
@@ -117,13 +117,15 @@ run_configure_and_make() {
     else
       ./configure $GAPDIR && $MAKE
     fi;
+  else
+    echo No building required for ${dir%/}
   fi;
 }

 for dir in `ls -d */`
 do
     if [ -e $dir/PackageInfo.g ]; then
-      echo ==== Building $dir
+      echo ==== Checking ${dir%/}
       case $dir in

@fingolfin
Copy link
Member Author

I think @alex-konovalov added that anupq build commands. Anyway, I now verified on two machines (one Ubuntu, one OS X) that correct 32 bit versions of anupq get built.

I added your improvements for the output of BuildPackages.sh. Also, there is one fixup commit (which I fill squash soon).

You had two remarks on improving FAQ items. While these sound valid, I'd rather not address them with this PR, simply because I want to get this merged ASAP, and I don't have the nerves to fret over the perfect formulation there right now...

@fingolfin fingolfin force-pushed the mh/improve-install branch from 018e0bc to 79ef037 Compare April 15, 2016 13:53
fingolfin and others added 6 commits April 15, 2016 15:53
New releases include the version in the directory name.
In particular, advertise bin/BuildPackages.sh and stop referencing
the old InstPackages.sh and InstPackages32.sh scripts.
I could not determine its purpose. In particular, 32 bit builds seem
to work fine without it.
@fingolfin
Copy link
Member Author

It would be great if people could test this, e.g. perhaps @alex-konovalov could give it a spin?

Once we merge this, we also should merge this back into master. And update http://www.gap-system.org/Download/tools.html suitably.

@olexandr-konovalov
Copy link
Member

@fingolfin @ChrisJefferson for ANUPQ, see gap-packages/anupq#17 - I was advised to always build it in 32-bit mode.

@fingolfin
Copy link
Member Author

@alex-konovalov I don't see how that answers anything -- it just contains a comment by you that you will from now on use that specific set of commands to compiler anupq, but it doesn't explain how you arrived at it... I vaguely recall giving some recommendations how compiling in 32bit mode might work, but I can't find it right now.

@olexandr-konovalov
Copy link
Member

@fingolfin I will check this, will plan to merge it next week (hopefully after current test failures will disappear)

@fingolfin
Copy link
Member Author

Here is one issue with BuildPackages.sh I just noticed (not sure if InstPackages.sh has the same problem or not): If I do as INSTALL.md suggests and build both a 32bit and 64bit version of GAP in the same source tree, and then also run BuildPackages.sh, like this:

  ./configure ABI=32 && make
  cd pkg && ../bin/BuildPackages.sh
  cd ..
  ./configure ABI=63 && make
  cd pkg && ../bin/BuildPackages.sh

Then this won't work quite right -- because for many packages (in particular: most (all?) that don't use autoconf), the build system won't detect that anything changed, and thus won't recompile the source files. So no 64 bit version is built, and/or there is a linker error.

Perhaps we should insert a "make clean", before doing make. But then we need to verify that this won't end up removing the 32 bit version.

@olexandr-konovalov
Copy link
Member

@fingolfin I assumed you know, because it was you who created that issue ;-) But I've tried to find some traces of that discussion on my machine, and can't find any too.

I think some time ago there was a problem with building ANUPQ on a 64-bit machine, and it was suggested to try to always enforce its 32-builds. Of course, we can drop that and then see if the problem was fixed by some other changes happening in the meantime in ANUPQ, or if it is still there...

@olexandr-konovalov
Copy link
Member

@fingolfin regarding buildibg both a 32bit and 64bit version of GAP in the same source tree - InstPackages.sh have the same problem.

@ChrisJefferson
Copy link
Contributor

Unfortunately, doing a make clean on many packages removes their pre-build documentation, which is in some cases we can't even rebuild. Therefore we can't just make clean all the time.

Of course, we should probably move towards make packages make clean-safe

@fingolfin
Copy link
Member Author

Ahhhh, now I understand (I think). Seems we've been talking past each other:

Yeah, there are (potential) issues with using anupq in 64 bit mode, and no, nobody worked on fixing them. So, that's what gap-packages/anupq#17 is about, and what I added to it.

However, the anupq special case in BuildPackages.sh I was referring to was doing something completely different; namely, it simple passed the CFLAGS etc. to make. Which made no sense to me. But now I see where it comes from; it likely was an incorrect attempt to add the "always build anupq in 32bit mode" logic.

So, now that this is clear, I can add back a new, "correct" special case for anupq. But first, I want to finally conduct some experiments to determine whether it is really necessary.

@olexandr-konovalov
Copy link
Member

I've checked this PR on my Ubuntu desktop and happy to merge it now. It will go into stable-4.8 where tests runs without diffs now, so we will now which change to blame :)

@olexandr-konovalov olexandr-konovalov merged commit 9fe68fc into gap-system:stable-4.8 Apr 15, 2016
@fingolfin fingolfin deleted the mh/improve-install branch August 18, 2016 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: build system topic: documentation Issues and PRs related to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants