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

tigerjython: init at 2.39 #316431

Merged
merged 2 commits into from
Aug 22, 2024
Merged

tigerjython: init at 2.39 #316431

merged 2 commits into from
Aug 22, 2024

Conversation

rcmlz
Copy link

@rcmlz rcmlz commented Jun 1, 2024

Description of changes

Adding new package TigerJython https://tigerjython.ch/

TigerJython offers everything you need to go from programming beginner to professional. You will find a wide variety of tutorials and can get started right away in programming environments specially developed for you.

TigerJython runs on the Java Virtual Machine (JVM), although we also offer the browser-based online solution WebTigerJython and WebTigerPython. The design and development of TigerJython follow the guidelines of simplicity, understandability, and—naturally—Python's battery included philosophy with an extensive set of libraries.

Things done

Taking the existing .tar.gz file from https://tigerjython.ch/en/products/download wrapping it to run the .jar file using the current stable JRE.

There are no dependencies introduced - just wrapping a jar for convenience.

I run nix-build -A tigerjython and then /nix/store/...-tigerjython-2.39/bin/tigerjython successfully.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jun 1, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 1, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jun 1, 2024
@Noodlez1232
Copy link
Contributor

Result of nixpkgs-review pr 316431 run on x86_64-linux 1

1 package failed to build:
  • tigerjython

@Noodlez1232
Copy link
Contributor

The failure here was a hash mismatch. Probably the solution here is to update the hash in the source for TigerJython.tar and for the tjlogo64.png files.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jun 2, 2024
@TomaSajt
Copy link
Contributor

TomaSajt commented Jun 5, 2024

I realized that this problem is not that uncommon across nixpkgs,
and a good solution I found was using http://web.archive.org/ to cache a download link.

I did that here: http://web.archive.org/web/20240119124245/https://tigerjython.ch/user/pages/download/TigerJython.tar.gz

You'll have to generate another snapshot when there's a new release, but this is much easier to maintain than a custom git repo.

@rcmlz
Copy link
Author

rcmlz commented Jun 6, 2024

That is a good idea. However, I am not sure if that approach is a stable enough solution for a 150MB download file.

However, I took your advice and automated the creation of the archive using Github Actions and put the docs for that on the front page of https://github.com/zero-overhead/tj.

That way we get booth a.) simple way to create an archive, b.) high availability and bandwidth in case a whole class of e.g. 24 pupils is running a nix-shell -p tigerjython simultaneously or, even better, uses the home-manager config supplied by the teacher ;-)

What do you think? Is that a way forward?

PS: I recon I could even extend this GH action to update the package.nix, test it and send a pull request. But that would be an optimization I would only consider later, and for sources that release frequently new versions and were it is important to be up to date.

TigerJython is very mature (and I assume mostly used in schools in the DACH area anyway). Using an older version is not a security risk - pupils just might not be able to connect the latest Robot or Platform or use the new actor X on model Y.

@rcmlz
Copy link
Author

rcmlz commented Jun 22, 2024

What can I do to make this PR moving forward?

@LeSuisse LeSuisse self-requested a review June 22, 2024 18:06
@emilazy
Copy link
Member

emilazy commented Jun 23, 2024

web.archive.org is used extensively throughout the tree (112 files in pkgs/ matching it), and doesn’t carry as many of the supply chain security risks as using a binary from an unreviewed maintainer‐controlled source. The Internet Archive regularly serves files of far greater size than that through their collections, and you can mark this package as unfreeRedistributable (“Any such service or product may be shared and distributed freely, provided the TJ Group is identified as the original copyright holder.”) to get nixos.org to cache it, so I think that’s a far preferable solution.

@rcmlz
Copy link
Author

rcmlz commented Jun 23, 2024

Thank you for your review. I was not aware that using archive.org is the standard way of doing it, furthermore I assumed that each installation will trigger a download from that side.

I changed the source accordingly - and use the same approach for a PR for another software (I like to be) used in schools - filius.

@TomaSajt
Copy link
Contributor

and you can mark this package as unfreeRedistributable (“Any such service or product may be shared and distributed freely, provided the TJ Group is identified as the original copyright holder.”) to get nixos.org to cache it, so I think that’s a far preferable solution.

AFAICT unfreeRedistributable packages are not built on hydra...
#83884

@emilazy
Copy link
Member

emilazy commented Jun 23, 2024

Sorry, you’re right; I got my wires crossed. Won’t tarballs.nixos.org still cache the source derivation though? I admit I haven’t spent much time working with non‐free packages, so I could be totally wrong here. In any case I’m pretty sure the Internet Archive can take the load.

@rcmlz
Copy link
Author

rcmlz commented Jun 24, 2024

Thank you for your help. I understood that the current state of the PR is as good as it gets? Can I improve something else? Do I need to edit any other files?

Copy link
Contributor

@TomaSajt TomaSajt left a comment

Choose a reason for hiding this comment

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

I've requested some changes.

Also, could you squash your commits into 2 commits?

  • one for adding yourself as a maintainer
  • and one for adding the package itself

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions
https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#commit-conventions

pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
@rcmlz
Copy link
Author

rcmlz commented Jun 24, 2024

Thank you. I incorporated the change requests. Can you give me a hint how I can squash the 3 commits into 2?

@TomaSajt
Copy link
Contributor

Thank you. I incorporated the change requests. Can you give me a hint how I can squash the 3 commits into 2?

You can run git reset HEAD~3 to undo the last 3 commits, but keep the changes. You can then create the two commits from there.
Follow the commit conventions I linked previously.

@rcmlz
Copy link
Author

rcmlz commented Jun 25, 2024

@TomaSajt @emilazy @Noodlez1232 @LeSuisse - Thank you all for your help - graciously appreciated. I learned a lot for my next contribution. Now I am eager to see how/if I can use Nix in school to ensure unified computer setup in a BYOD world.

Copy link
Contributor

@vigress8 vigress8 left a comment

Choose a reason for hiding this comment

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

Per NixOS/rfcs#166, format the file with

nix run nixpkgs#nixfmt-rfc-style -- pkgs/by-name/ti/tigerjython/package.nix

pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
@vigress8
Copy link
Contributor

Since updating this seems somewhat cumbersome, I wonder if an updateScript could be written to handle it.

@rcmlz
Copy link
Author

rcmlz commented Jun 26, 2024

@vigress8 thank you for your review. I included the suggested changes (and fixed a small bug in the java wrapper).

With regards to the suggested update script as a side note: TigerJython is mature and not releasing frequently. I included the steps a human (me) or a script would need to do.

Do you assume scraping https://tigerjython.ch/en/products/download for version string and download link is stable enough to implement?

My current view is that the risk of failures in webscraping is higher than the effort of a yearly (before Autumn term) manual update.

@vigress8
Copy link
Contributor

My current view is that the risk of failures in webscraping is higher than the effort of a yearly (before Autumn term) manual update.

Fair enough, it's not quite fit for automation. But perhaps a script meant to run manually, and the result checked by a human, would be useful.

Copy link
Contributor

@TomaSajt TomaSajt left a comment

Choose a reason for hiding this comment

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

The PR looks pretty complete now, though I still have some small suggestions.

pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tigerjython/package.nix Outdated Show resolved Hide resolved
@TomaSajt
Copy link
Contributor

Also I'm a bit concerned about the long download times.
If someone used nix-collect-garbage, the tarball would probably get cleared out, since it's a build-time only dependency. Then, if the package needed to be rebuilt, it would need to fetch the tarball again, which is painfully long.
Let's hope whoever uses this package uses the stable branches, where things don't get rebuilt so often.

It might be possible to hack together something where we symlink the tarball's files instead of copying them into the main package. This way they should not get cleared. What do you think, is it worth trying?

@rcmlz
Copy link
Author

rcmlz commented Jun 26, 2024

It might be possible to hack together something where we symlink the tarball's files instead of copying them into the main package. This way they should not get cleared. What do you think, is it worth trying?

I am not able to hack together such a thing. Would the generic solutions to this situation not be - as suggested by you - to cache unfreeRedistributable packages? This was the Issue you linked.

I personally plan to use only the stable branch (School - things must work in class, no experiments on infrastructure during terms)

Alternatively I ask upstream if I can declare the package with a free license such that caching happens automatically?

@TomaSajt
Copy link
Contributor

I've though about the download speed a bit more and I think it's fine as it is for now.

If someone has an issue with long download times when rebuilding, we can ask them to just manually download the tar.gz file and do (tigerjython.overrideAttrs { src = ./TigerJython.tar.gz; })

Copy link
Contributor

@TomaSajt TomaSajt left a comment

Choose a reason for hiding this comment

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

LGTM for the most part.
If some issue comes up in the future, we can fix it then.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 10, 2024
@JohnRTitor JohnRTitor changed the title Adding TigerJython tigerjython: init at 2.39 Jul 12, 2024
@JohnRTitor
Copy link
Contributor

Result of nixpkgs-review pr 316431 run on x86_64-linux 1

1 package built:
  • tigerjython

@rcmlz
Copy link
Author

rcmlz commented Aug 5, 2024

@LeSuisse - what do you think? Is this PR ready? To start TigerJython currently I use

export NIXPKGS_ALLOW_UNFREE=1; nix run github:nixos/nixpkgs/pull/316431/head#tigerjython --extra-experimental-features "nix-command flakes" --impure

but it would be more convinient to have it as a standard package.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

I’m sorry this stalled out. I couldn’t merge things back when I left my original comments but I can now. This looks good to me and I’m happy to add it as‐is. Thank you for your contribution!

@emilazy emilazy merged commit e4966e3 into NixOS:master Aug 22, 2024
27 checks passed
@donovanglover donovanglover mentioned this pull request Aug 28, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants