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

guile-sdl2: 0.1.0 -> 0.2.0 #29887

Merged
merged 5 commits into from
Sep 28, 2017
Merged

guile-sdl2: 0.1.0 -> 0.2.0 #29887

merged 5 commits into from
Sep 28, 2017

Conversation

vyp
Copy link
Member

@vyp vyp commented Sep 28, 2017

Motivation for this change
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.

cc @seppeljordan

Autoconf, automake, and running `./bootstrap` is no longer required
because we are now building from the tarball, which includes the
generated ./configure script.
let
name = "${pname}-${version}";
pname = "guile-sdl2";
version = "0.2.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you bring these into mkDerivation for consistency with other derivations?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I put it outside so that mkDerivation doesn't need rec. Which other derivations are you talking about?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to this approximate count, most of them:

$ ag -G '\.nix$' -l '(?s)version =.*mkDerivation' | wc -l
678
$ ag -G '\.nix$' -l '(?s)mkDerivation rec .*version =' | wc -l
3494

and, in particular, the previous PR #29886. There is no need to remove rec from mkDerivation.

Copy link
Member Author

Choose a reason for hiding this comment

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

But wouldn't you say it's better without rec? I'm bit hesitant because this would mean it's inconsistent with most other guile-modules that do not need to use rec: 3579d7e

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess my point is why use rec when it's not needed?

Copy link
Member Author

@vyp vyp Sep 28, 2017

Choose a reason for hiding this comment

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

#29886 doesn't remove rec because it needs it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that in general these attrs are put into mkDerivation because rec version is shorter, and they can be used from other derivations — for example, grep .version} in the repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that seems really useful, I didn't think about that.

So I adopted this style after seeing it somewhere, mainly just because of version and how putting it in let can remove the need for rec in a lot of cases. And otherwise if this was not done, rec seemed to nearly always be needed, which didn't seem right.

Anyway in that case I'll switch to setting version too in mkDerivation, thanks for mentioning this. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not have strong feelings about the rec but just so you know: there is a function called builtins.parseDrvName to get version info from derivation name. Which makes putting version into the derivation a second time not necessary. Because parseDrvName should work with far more derivations than asking for a version attribute using the builtin function would be more desirable anyway, at least in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@seppeljordan Oh right that's even better, thanks for letting me know. :)

"--with-libsdl2-mixer-prefix=${SDL2_mixer}"
];

makeFlags = [ "GUILE_AUTO_COMPILE=0" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not appear to do anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

It just gets rid of compilation warnings/errors such as (WARNING, ERROR, etc):

;;; note: auto-compilation is enabled, set GUILE_AUTO_COMPILE=0
;;;       or pass the --no-auto-compile argument to disable.
;;; compiling /nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild
;;; WARNING: compilation of /nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild failed:
;;; ERROR: failed to create path for auto-compiled file "/nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild"
  GEN      sdl2/config.go
wrote `sdl2/config.go'
;;; note: auto-compilation is enabled, set GUILE_AUTO_COMPILE=0
;;;       or pass the --no-auto-compile argument to disable.
;;; compiling /nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild
;;; WARNING: compilation of /nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild failed:
;;; ERROR: failed to create path for auto-compiled file "/nix/store/1djqc42mqb1hacj40228cm2wq50i8c1d-guile-2.2.0/bin/guild"

I can still remove it though if you'd like?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like it prevents Guile from doing unnecessary things so it's useful to an extent, in my opinion, but idm not having it either.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, I did not notice this because there were no warnings when building in a terminal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I was building in a terminal too (not emacs or anything if that's what you mean?), strange. 😕

@orivej orivej merged commit 637c105 into NixOS:master Sep 28, 2017
@vyp vyp deleted the upd/guile-sdl2 branch September 28, 2017 10:41
@seppeljordan
Copy link
Contributor

@vyp thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants