pharo: 5.0 -> 6.0 #26924

lukego · 2017-06-28T09:32:28Z

Update Pharo for the 6.0 release. This includes a new upstream VM package, with a new build procedure, and separate 32-bit/64-bit variants.

This branch mostly done but with a little work still in progress:

Create new set of VM packages:
- pharo-cog32: 32-bit VM for legacy cog images.
- pharo-spur32: 32-bit VM for Pharo 6 (and late Pharo 5.)
- pharo-spur64: 64-bit VM for Pharo 6 (and late Pharo 5.) Experimental!
- pharo: Multi-VM wrapper that automatically selects a suitable VM based on the image being opened.
Upgrade spur VM to the new OpenSmalltalk version and build process. This is the standard one now.
Upgrade spur VM source to the latest version recommended by upstream.
Itegrate improvements from pharo-vm: Add third-party libraries to LD_LIBRARY_PATH #25681.
Mark pharo-launcher as unmaintained. (Seems to require coordinating with upstream developers, who have some multi-VM selection logic on the Smalltalk side, and on reflection I don't have time for this.)

cc @Balletie

Motivation for this change

Support the latest Pharo VM in order to run the latest Pharo images.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Create a new set of VM packages to keep up with changes in the upstream Pharo project.

0xABAB · 2017-06-28T14:16:41Z

pkgs/development/pharo/vm/build-vm-legacy.nix

+    mv vm-sound-ALSA vm-sound-ALSA.so
+    mv pharo pharo-vm
+
+    cp * "$prefix/lib/$name"


Better to use ./*.

0xABAB · 2017-06-28T14:18:13Z

pkgs/development/pharo/vm/build-vm.nix

-  };
+  # Choose target platform name in the format used by the vm.
+  flavor =
+    if      stdenv.isLinux && stdenv.isi686    then "linux32x86"


This can be factored, but perhaps it was a conscious choice. No need to reply, just consider it.

0xABAB · 2017-06-28T14:18:45Z

pkgs/development/pharo/vm/build-vm.nix

+  # Regenerate the configure script.
+  # Unnecessary? But the build breaks without this.
+  autoreconfPhase = ''
+    (cd opensmalltalk-vm/platforms/unix/config && make)


What's up with the subshell?

The subshell was there to isolate the change of directory. I switched from (cd x && foo) to pushd x; foo; popd style which is hopefully less controversial.

@lukego A subshell is great if you want to accomplish that (and certainly better than the replacement with pushhd and popd), but I am and wasn't sure whether the different phases run within the same shell process and whether or not they run from the same directory. I wouldn't expect the phase's execution semantics to be dependent on the other phases having run, which is what you seem to imply. If this is true, then I would consider it a design flaw. So, are you absolutely sure that you need it?

0xABAB · 2017-06-28T14:20:19Z

pkgs/development/pharo/vm/build-vm.nix

-    EOF
+  # Note: --with-vmcfg configure option is broken so copy plugin specs to ./
+  preConfigure = ''
+    cd opensmalltalk-vm


Please rewrite without using the cd command. I.e. add its argument to the paths in the next line.

0xABAB · 2017-06-28T14:20:57Z

pkgs/development/pharo/vm/build-vm.nix

+  # Note: --with-vmcfg configure option is broken so copy plugin specs to ./
+  preConfigure = ''
+    cd opensmalltalk-vm
+    cp build.${flavor}/pharo.cog.spur/plugins.{ext,int} .


Quote ${flavor}, even though it now isn't needed.

0xABAB · 2017-06-28T14:21:54Z

pkgs/development/pharo/vm/build-vm.nix

+    make install-squeak install-plugins prefix=$(pwd)/products
+
+    # Copy binaries & rename from 'squeak' to 'pharo'
+    mkdir -p $out


$out can contain spaces with a different store location, right? So, that needs to be quoted.

0xABAB · 2017-06-28T14:22:27Z

pkgs/development/pharo/vm/build-vm.nix

+
+    # Copy binaries & rename from 'squeak' to 'pharo'
+    mkdir -p $out
+    cp    products/lib/squeak/5.0-*/squeak $out/pharo


Inconsistent spacing.

0xABAB · 2017-06-28T14:23:27Z

pkgs/development/pharo/vm/build-vm.nix

+    # LD_LIBRARY_PATH. This is important because various C code in the VM
+    # and Smalltalk code in the image will search for them there.
+    mkdir -p $out/bin
+    chmod u+w $out/bin


I'd prefer non-differential rights. I.e. chmod 0XXX for some values of XXX.

0xABAB · 2017-06-28T14:25:48Z

pkgs/development/pharo/vm/wrapper.sh

+
+# Run the VM
+set -f
+exec ${vm} $args "$@"


${vm} also needs quotes.

0xABAB · 2017-06-28T14:26:58Z

pkgs/development/pharo/wrapper/pharo-vm.sh

+
+# Run the VM
+set -f
+exec -- ${vm} $@


Quoting again and also add -- to the other exec invocation.

0xABAB

Fix the quoting issues everywhere (some issues I haven't marked multiple times), and the other comments, and I am happy.

Balletie · 2017-06-28T21:36:14Z

pkgs/development/pharo/vm/build-vm.nix

-    ln -s "${pharo-share}/lib/"*.sources $prefix/lib/$name
-  '';
+  # Note: Force gcc6 because gcc5 crashes when compiling the VM.
+  buildInputs = [ bash unzip glibc openssl gcc6 mesa freetype xorg.libX11 xorg.libICE xorg.libSM alsaLib cairo pharo-share libuuid autoreconfHook ];


Some of these are only build-time dependencies, e.g. unzip, gcc. Those should be listed as nativeBuildInputs instead, since buildInputs is meant for runtime dependencies

FYI I've done this in my small PR (#25681), but maybe bash should be a runtime dependency.

Balletie · 2017-06-28T21:39:08Z

pkgs/development/pharo/vm/build-vm-legacy.nix

+
+{ name, src, ... }:
+
+stdenv.mkDerivation rec {


Is there a use case for keeping the legacy expression around? If someone really needs it it's still there in previous revisions of nixpkgs.

Balletie · 2017-06-28T21:44:21Z

pkgs/development/pharo/vm/build-vm.nix

    EOF
+    chmod +x $prefix/bin/${cmd}


Use makeWrapper instead of this script (see my changes in #25681). It essentially does the same thing but it just makes it a bit more maintainable.

lukego · 2017-06-29T18:12:42Z

@0xABAB Thanks for the reviews! I have pushed some fixes intended to address them. Aside: I would find it helpful if the Nixpkgs Contributors Guide would provide a rationale for these somewhere. I don't immediately understand the difference between * and ./* or why it is necessary to quote nix store paths.

lukego · 2017-06-29T18:12:59Z

@Balletie Could you be tempted to rebase your changes on this branch and PR them?

lukego · 2017-06-29T18:14:53Z

I spent a little time trying to use multiple outputs to reduce the amount of symlinkage into the environment. I am not sure why but I didn't make that work. I intended to leave that as-is for the purposes of this PR and I don't think that it poses a practical problem. Could be improved in the future when I have more experience with multiple outputs.

lukego · 2017-06-29T18:18:15Z

@Balletie Good question about use cases. It's important since this package only provides the VM which is not useful unless you combine it with an image. So where does the image come from, and how do you make sure the image and VM are compatible?

My premise on this branch is that you are downloading images from the internet, e.g. Pharo 5, Pharo 6, Pharo 7 preview, Moose 6, Moose 6.1, DrGeo, etc, and you often have no special knowledge of which VM is required (and don't care - you just want to run the image.) So in this case the wrapper script does the right thing by making sure all VMs are available and selecting the appropriate one.

(Could be that those images are often distributed together with "appropriate" VMs but those will be binaries that you can't simply run on NixOS out of the box.)

Note: There are many Pharo images in the wild, and each one works with only one of the cog32/spur32/spur64 variants, so any time we drop a VM variant we are limiting the set of images that can be opened.

Balletie · 2017-06-29T20:35:50Z

I'll try to rebase tomorrow.

Regarding the comment about the use case: I was trying to say that maybe we should remove the build-vm-legacy.nix file, since it won't be used anymore after your changes, no?

Nice work on detecting the appropriate VM for any given image by the way. Something similar was also present in the pharo launcher (or still is?) when the migration from cog to spur happened.

lukego · 2017-06-30T07:33:03Z

maybe we should remove the build-vm-legacy.nix file, since it won't be used anymore after your changes, no?

Oops! I actually missed committing the file where it is referenced, vms.nix. Added in 4ed1d53. Guess I need to start paying attention to the Travis result. Had assumed the red was a false-negative.

lukego · 2017-06-30T08:10:04Z

I pushed an update to the latest recommended version of the spur VM (based on feedback on the pharo-dev mailing list since upstream are not making actual source releases nowadays.) The 64-bit VM is now not working properly with my usual test image but I propose to simply live with this for now, since 64-bit support is still an experimental feature and I don't know how stable and compatible it is supposed to be. Hope to fix it with a future PR.

lukego · 2017-06-30T08:12:16Z

I decided to skip testing the desktop integration (icons, open on double-click, etc.) My NixOS desktop is not working that well - can't open a file browser in xfce for some reason - and I don't have the time to troubleshoot that (I don't use any of that stuff myself anyway.)

lukego · 2017-06-30T08:19:42Z

Travis-CI is reporting a build error but I don't understand its report. Can somebody please help me to decipher it?

This commit adds the third party libraries needed by the default Pharo environment to the LD_LIBRARY_PATH, by using makeWrapper.

Balletie · 2017-06-30T12:10:31Z

@lukego You can cherry pick from my pharo6-for-nixpkgs branch (it's this commit: Balletie@96600d5). That way this PR can stay open.

0xABAB

It's not perfect, but it's good enough.

lukego · 2017-07-03T07:55:34Z

Thanks @0xABAB for the approval. I think it makes sense to land imperfect code and improve it incrementally. (I've already spent about a week on this, spread out over a couple of months, trying to understand what has been happening with the upstream project and coordinate with their release plans. And there is still a lot of that work to do in order to create Pharo images within Nix. Hurts a bit :))

@Balletie I merged your branch, thanks! Sorry about the wait, I did not immediately realize that you had created a new branch with the changes rebased. Thank you for that.

Is this branch ready to merge then? I am fine with any rebasing/squashing/etc that anybody wants to do / wants me to do.

This is important. The VM was not compiled in "Pharo mode" and this made certain primitives return different values to the Smalltalk side. (Practically speaking I am surprised that the VM has been working basically fine for me for weeks, but this resolves a problem with adding filetree:// repositories with Monticello that failed in an obscure way when the file permissions lookup primitive did not behave as expected.) See also: https://pharo.fogbugz.com/f/cases/20217/Image-does-not-detect-incompatible-opensmalltalk-vm

Building with GCC > 4.9 produces a broken VM for reasons that are not yet understood, see http://forum.world.st/OSProcess-fork-issue-with-Debian-built-VM-td4947326.html also disable "stackprotector" hardening for compatibility with this older gcc.

lukego · 2017-07-10T04:01:55Z

... Since this PR is still open I will continue to push my Pharo related fixes here. The latest is a downgrade to gcc 4.8 to work-around a segfault crash when forking a subprocess (2b3dcfa
). Have to revisit this when upstream work out what is happening there.

I have reached out for help with identifying a suitable test suite for the VM to find this problems more proactively.

0xABAB · 2017-07-10T07:27:22Z

@lukego If you believe the packaging is done, just contact e.g. Mic92 with a request to merge (I already thought it was good enough to merge earlier).

The reason Pharo crashes with newer compilers is because newer compilers tend to exploit C semantics, and in particular undefined behavior, better. Pharo programmers used fork without understanding what it does (next to some other out of bounds issue being present).

So, while many people would traditionally point at the compiler for being wrong with the argument that the old compiler did work, this line of reasoning is false. I fully expect the same problems on other platforms, if not today, then tomorrow.

I would not recommend anyone to run anything important on Pharo, because of the referenced bug.

In our packaging it would be nice to have a marker like "known to exhibit nasal daemons for programs with known undefined behavior", but unfortunately I expect that most people in this community would consider such a feature to be too harsh.

lukego · 2017-07-10T07:44:41Z

Closing in a huff because I am annoyed by the unsolicited disparaging comments about the software that I am packaging.

lukego · 2017-07-10T09:21:31Z

@0xABAB Please bear with me for a bit. I am frustrated because I don't understand the process for contributing to nixpkgs (what is a "merge request"? who is "Mic92"?) and I don't enjoy the feedback you are giving me (feels a bit gratuitously negative to me and I am not sure if/how I am supposed to respond in this context i.e. on a PR that I have open.)

I am just looking for the lowest friction way to share the work that I am doing on packaging Pharo in order to avoid duplication by other people who want to use it too.

eqyiel · 2017-07-10T23:22:01Z

@lukego please don't be discouraged!

#27258 (comment)

dtzWill · 2017-07-11T13:23:20Z

pkgs/development/pharo/launcher/default.nix

@@ -52,7 +51,7 @@ stdenv.mkDerivation rec {
     secs=5
     echo -n "Running headless Pharo for $secs seconds to check for a crash... "
     timeout $secs \
-       ${pharo-vm}/bin/pharo-vm-nox PharoLauncher.image --no-quit eval 'true'
+       "${pharo}/bin/pharo" -nodisplay PharoLauncher.image --no-quit eval 'true'


Possible minor typo: At least on my machine, pharo complains about the -nodisplay argument, using two dashes (--nodisplay) does the trick.

lukego · 2017-07-11T13:51:30Z

I understand from #27298 that this branch may be rebased/squashed on the way into the repo. That's fine.

Closes #26924

joachifm · 2017-08-26T13:20:22Z

@lukego merged. Thank you

nixos-discourse · 2019-02-14T23:18:23Z

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/code-of-conduct-or-whatever/2134/1

pharo: 5.0 -> 6.0

fdb90f6

Create a new set of VM packages to keep up with changes in the upstream Pharo project.

lukego mentioned this pull request Jun 28, 2017

pharo-vm: Add third-party libraries to LD_LIBRARY_PATH #25681

Closed

7 tasks

0xABAB reviewed Jun 28, 2017

View reviewed changes

pkgs/development/pharo/vm/wrapper.sh Outdated

# Run the VM

set -f

exec ${vm} $args "$@"

Copy link

Contributor

0xABAB Jun 28, 2017

Choose a reason for hiding this comment

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

${vm} also needs quotes.

0xABAB reviewed Jun 28, 2017

View reviewed changes

0xABAB suggested changes Jun 28, 2017

View reviewed changes

Balletie reviewed Jun 28, 2017

View reviewed changes

lukego added 2 commits June 29, 2017 18:09

pharo6: Minor fixes and cleanups

54be67d

pharo: Removed obsolete duplicate ofwrapper.sh

8761ef2

lukego added 2 commits June 30, 2017 07:24

pharo: More quoting

86d3b59

pharo: Add missing file: vms.nix

4ed1d53

pharo: Update build-vm.nix for latest upstream

d55f827

pharo-vm: Add third-party libraries to LD_LIBRARY_PATH of legacy VM

96600d5

This commit adds the third party libraries needed by the default Pharo environment to the LD_LIBRARY_PATH, by using makeWrapper.

0xABAB approved these changes Jul 1, 2017

View reviewed changes

pSub added the 8.has: package (update) label Jul 3, 2017

lukego added 2 commits July 5, 2017 04:53

pharo: Add PharoV60.sources to support newer images

448fe16

lukego changed the title ~~pharo: 5.0 -> 6.0 [work in progress]~~ pharo: 5.0 -> 6.0 Jul 5, 2017

pharo: build with gcc48

2b3dcfa

Building with GCC > 4.9 produces a broken VM for reasons that are not yet understood, see http://forum.world.st/OSProcess-fork-issue-with-Debian-built-VM-td4947326.html also disable "stackprotector" hardening for compatibility with this older gcc.

lukego closed this Jul 10, 2017

lukego mentioned this pull request Jul 11, 2017

When to rebase, and when not to? #27298

Closed

Profpatsch mentioned this pull request Jul 11, 2017

Cannot comment anymore #27244

Closed

vcunat reopened this Jul 11, 2017

dtzWill reviewed Jul 11, 2017

View reviewed changes

lukego mentioned this pull request Jul 11, 2017

How to maintain a long-lived custom nixpkgs tree? #27312

Closed

domenkozar approved these changes Jul 12, 2017

View reviewed changes

joachifm merged commit 2b3dcfa into NixOS:master Aug 26, 2017

joachifm added a commit that referenced this pull request Aug 26, 2017

Merge remote-tracking branch 'lukego/pharo6-for-nixpkgs'

5ddacdc

Closes #26924

pharo: 5.0 -> 6.0 #26924

pharo: 5.0 -> 6.0 #26924

Conversation

lukego commented Jun 28, 2017 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xABAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Balletie Jun 28, 2017 • edited

Choose a reason for hiding this comment

Balletie Jun 28, 2017 • edited

Choose a reason for hiding this comment

lukego commented Jun 29, 2017

lukego commented Jun 29, 2017

lukego commented Jun 29, 2017

lukego commented Jun 29, 2017

Balletie commented Jun 29, 2017 • edited

lukego commented Jun 30, 2017

lukego commented Jun 30, 2017

lukego commented Jun 30, 2017

lukego commented Jun 30, 2017

Balletie commented Jun 30, 2017 • edited

0xABAB left a comment

Choose a reason for hiding this comment

lukego commented Jul 3, 2017

lukego commented Jul 10, 2017

0xABAB commented Jul 10, 2017

lukego commented Jul 10, 2017

lukego commented Jul 10, 2017

eqyiel commented Jul 10, 2017

Choose a reason for hiding this comment

lukego commented Jul 11, 2017

joachifm commented Aug 26, 2017

nixos-discourse commented Feb 14, 2019

lukego commented Jun 28, 2017 •

edited

Balletie Jun 28, 2017 •

edited

Balletie Jun 28, 2017 •

edited

Balletie commented Jun 29, 2017 •

edited

Balletie commented Jun 30, 2017 •

edited