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

pharo: old -> 10.0.5 #237892

Merged
merged 1 commit into from
Aug 3, 2023
Merged

pharo: old -> 10.0.5 #237892

merged 1 commit into from
Aug 3, 2023

Conversation

jthulhu
Copy link
Contributor

@jthulhu jthulhu commented Jun 15, 2023

Description of changes

The Pharo packages were waaaaay outdated (~10 years old). This is a first attempt to update the packages.

  • Pharo launcher: the launcher has been deleted from the repo, but hasn't been updated, because the version previously packaged is not compatible with the newly packaged Pharo (at least, I haven't been able to make it work)
  • Pharo VMs:
    • Spur 64: this has been packaged to version 10.0.5 (which is the latest version available, not to be confused with the version of the Pharo image).
    • Spur 32: this hasn't been packaged; it shouldn't be hard (just changing the source url and the checksum), but it hasn't been done.
    • Cog 32: this is obsolete, so it has been entirely removed.

For this reason, the availability of the Pharo packaged has regressed (only linux x86_64 is supported in this PR).

While this builds and works, any attempt at using Cairo will throw an exception. This should be fixed.

Important: the previous maintainer has been removed from the maintainer list, since this packaged was clearly not maintained anymore.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-download-from-url-while-packaging-for-nixpkgs/29159/1

@jthulhu
Copy link
Contributor Author

jthulhu commented Jun 16, 2023

Cairo support has been added and tested.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for your contribution.

I've left some comments, left me know if this is clear enough.

pkgs/development/pharo/default.nix Outdated Show resolved Hide resolved
pkgs/development/pharo/default.nix Show resolved Hide resolved
pkgs/development/pharo/default.nix Outdated Show resolved Hide resolved
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

@drupol the requested changes were pretty straightforward (thanks for the examples!).

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the changes.

Last thing, could you sort alphabetically the lambda parameters, the buildInputs, nativeBuildInputs and the meta attributes ?

Don't forget to squash your commits so that this PR only contains 1 single commit.

pkgs/development/pharo/default.nix Outdated Show resolved Hide resolved
@jthulhu jthulhu force-pushed the update-pharo branch 2 times, most recently from fd69ff8 to b16242e Compare August 3, 2023 07:06
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

I have sorted the field, changed the platform and squashed into a single commit.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I forgot one last nit.

I usually recommend to use makeBinaryWrapper by default since it has a much better compatibility with Darwin platforms.

pkgs/development/pharo/default.nix Outdated Show resolved Hide resolved
pkgs/development/pharo/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 3, 2023
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

I have replaced makeWrapper with makeBinaryWrapper, but currently this package is not darwin-compatible, because it explicitly fetches sources that are made for Linux (although this is pretty easy to fix).

The reason there is a distinction is that Pharo needs to bootstrap itself (Pharo is written in Pharo), and to do so without an available VM the CI provides a translation of the Pharo VM in C; and their translation is target-specific, AFAIK.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM now.

I see that you removed a bunch of entries (pharo-vms, pharo-cog32, etc etc)... shoudln't you add something in aliases.nix to let the users know that they doesn't exist anymore? And eventually a message in the release note ?

@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

I have added the removed packages to aliases.nix, although the previous version was so severely outdated that I doubt anyone was still using it...

Besides that, how can I add a message in the release note?

@drupol
Copy link
Contributor

drupol commented Aug 3, 2023

Here's an example on how to add an entry in the release note.

The file you need to edit is nixos/doc/manual/release-notes/rl-2311.section.md

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Aug 3, 2023
@drupol
Copy link
Contributor

drupol commented Aug 3, 2023

Could you please rebase your PR so there's only one commit?

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 3, 2023
@ofborg ofborg bot removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-linux: 1 labels Aug 3, 2023
@jthulhu jthulhu force-pushed the update-pharo branch 2 times, most recently from 4180a3f to 468067d Compare August 3, 2023 13:56
@github-actions github-actions bot added 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 3, 2023
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

@drupol I'm not sure if I properly rebased, now it looks like it wants to merge 68 commits... I merged from master, then I rebased to move my commit to the end.

@RaitoBezarius
Copy link
Member

@drupol I'm not sure if I properly rebased, now it looks like it wants to merge 68 commits... I merged from master, then I rebased to move my commit to the end.

You failed your rebase and pinged other people, please remove those commits.

@github-actions github-actions bot removed 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 3, 2023
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

I'm not too sure how to resolve this merge. If I simply merge, that makes 2 commits and git refuses to amend the previous commit. If I try to rebase, it forcefully inserts commits from master into my branch.

@RaitoBezarius
Copy link
Member

I'm not too sure how to resolve this merge. If I simply merge, that makes 2 commits and git refuses to amend the previous commit. If I try to rebase, it forcefully inserts commits from master into my branch.

Read the CONTRIBUTING guide on mass pings and retargetting, it is relevant to the problem I imagine.

@drupol
Copy link
Contributor

drupol commented Aug 3, 2023

Hi,

Here's a couple of steps that should help you fixing the current issue:

  1. cd /home/.../nixpkgs
  2. git checkout master
  3. git remote add upstream [email protected]:NixOS/nixpkgs.git
  4. git fetch --all
  5. git reset upstream/master --hard
  6. git push
  7. git checkout update-pharo
  8. git rebase master
  9. git push -f

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 3, 2023
@jthulhu
Copy link
Contributor Author

jthulhu commented Aug 3, 2023

Is there a way to unping the two contributors that were accidentally added as reviewers?

@jthulhu jthulhu mentioned this pull request Aug 3, 2023
@mweinelt mweinelt removed their request for review August 3, 2023 15:18
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM !

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 3, 2023
@drupol drupol merged commit 9aabdcf into NixOS:master Aug 3, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants