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.

Sign up for GitHub

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

Jump to bottom

aspectj: 1.5.2 -> 1.9.6 #96209

Merged
merged 1 commit into from Aug 25, 2020
Merged

aspectj: 1.5.2 -> 1.9.6 #96209

merged 1 commit into from Aug 25, 2020

Conversation

bobismijnnaam
Copy link
Contributor

Motivation for this change

The package version was quite behind the current stable release. Also, as discussed on the mailing list (see here), the url is now pointed at the github project release because the previous eclipse archive link was not kept up-to-date anymore.

I have tested the new version with Larva runtime verification tool and there were no problems.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions (Ubuntu)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

danieldk
danieldk suggested changes Aug 25, 2020
View reviewed changes
pkgs/development/compilers/aspectj/default.nix Outdated
name = "aspectj-1.5.2";
pname = "aspectj";
version = "1.9.6";
versionSnakeCase = builtins.replaceStrings ["."] ["_"] version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
versionSnakeCase = builtins.replaceStrings ["."] ["_"] version;

Use something like

  src = let
    versionSnakeCase = builtins.replaceStrings ["."] ["_"] version;
  in fetchurl {
    url = "http://archive.eclipse.org/tools/aspectj/${name}.jar";
    sha256 = "1b3mx248dc1xka1vgsl0jj4sm0nfjsqdcj9r9036mvixj1zj3nmh";
    url = "https://github.com/eclipse/org.aspectj/releases/download/V${versionSnakeCase}/aspectj-${version}.jar";
    sha256 = "02jh66l3vw57k9a4dxlga3qh3487r36gyi6k2z2mmqxbpqajslja";
  };

We should avoid adding unnecessary environment variables to the build environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied the suggestion.

@bobismijnnaam
aspectj: 1.5.2 -> 1.9.6
258fe58 
As discussed on the mailing list, the url is now pointed at the github
project release because the previous eclipse archive link was not kept
up-to-date anymore.

Relevant links:
- https://www.eclipse.org/lists/aspectj-dev/msg03311.html
danieldk
danieldk approved these changes Aug 25, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

The version bump looks ok.

Result of nixpkgs-review pr 96209 1

1 package built:
- aspectj

Note though that this derivation needs work, because it is impure. E.g., it requires /bin/sh to be available.

@danieldk danieldk merged commit b9ba82e into NixOS:master Aug 25, 2020
@bobismijnnaam bobismijnnaam deleted the aspectj-upgrade-2 branch August 25, 2020 09:00
@bobismijnnaam
Copy link
Contributor Author

Note though that this derivation needs work, because it is impure. E.g., it requires /bin/sh to be available.

@danieldk Is this because of the usage of $SHELL in builder.sh? Or because builder.sh has no shebang? What other things in this derivation make it impure?

@danieldk
Copy link
Contributor

danieldk commented Aug 25, 2020 •
edited

Note though that this derivation needs work, because it is impure. E.g., it requires /bin/sh to be available.

@danieldk Is this because of the usage of $SHELL in builder.sh? Or because builder.sh has no shebang? What other things in this derivation make it impure?

Sorry, I meant that the output is impure. Most of the scripts (e.g. $out/bin/aj) use /bin/sh in their shebang, which means that they do not work in environments where /bin/sh is not available. patchShebangs should be applied to the scripts to solve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@bobismijnnaam @danieldk