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

dotenv: fix broken package #73256

Merged
merged 6 commits into from Nov 12, 2019
Merged

Conversation

@cptrodolfox
Copy link
Contributor

cptrodolfox commented Nov 11, 2019

Motivation for this change
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
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • [x ] Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • [x ] 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.
Notify maintainers

cc @

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 11, 2019

@cptrodolfox Thanks for sending this PR.

However, there are a few things that need to be changed before this can be merged.

Specifically, 2. and 3. from this comment: #72923 (comment)

@cptrodolfox

This comment has been minimized.

Copy link
Contributor Author

cptrodolfox commented Nov 12, 2019

Hi @cdepillabout, thank you for the comment. But, I have one question to fix this package I had to use the newest version from Hackage. The error that caused this package to be broken came from upstream. That, It is why I had to modify configuration-hackage2nix.nix, to update the versions of the dotenv package, the megaparsec package, and the hspec-megaparsec package. If I can not modify this file, should I wait for when the update is ready here?

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 12, 2019

@cptrodolfox I'm not quite sure I follow you.

So dotenv-0.8.0.2 (the current version in nixpkgs), is completely broken and there is no way to fix it?

Even with your following fix?

dotenv = addBuildTool super.dotenv pkgs.buildPackages.coreutils;
@cptrodolfox

This comment has been minimized.

Copy link
Contributor Author

cptrodolfox commented Nov 12, 2019

Not really, If you put dontCheck it will be fixed I think. Because, the build only fails in the test part.

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 12, 2019

@cptrodolfox Ah okay, I see.

Could you update this PR to just mark dotenv as dontCheck, and make sure to keep your change where you remove it from the broken-packages list?

Please remove the other changes from this PR (updating pkgs/development/haskell-modules/hackage-packages.nix, and the default-package-overrides list).

@cptrodolfox cptrodolfox force-pushed the stackbuilders:dotenv-fix branch from 266a662 to 591da0c Nov 12, 2019
@cptrodolfox

This comment has been minimized.

Copy link
Contributor Author

cptrodolfox commented Nov 12, 2019

Hi @cdepillabout , I added dontCheck to the dotenv package (dotenv 0.8.0.2). I observed that a new package is available in package-packages (dotenv_0_8_0_4). Why did this happen?
Pd: Sorry, I didn't update the PR yesterday it was late here.

William R. Arellano
@cptrodolfox cptrodolfox changed the title dotenv: fix broken package and updated megaparsec dotenv: fix broken package Nov 12, 2019


# Tests fails because of missing test file
dotenv = dontCheck super.dotenv;

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 12, 2019

Member

@cptrodolfox This looks good except for one thing: This override should actually be in the configuration-common.nix file.

Once you move it there I will go ahead and merge it in!

This comment has been minimized.

Copy link
@cptrodolfox

cptrodolfox Nov 12, 2019

Author Contributor

Thanks, I just moved the change.

This comment has been minimized.

Copy link
@cptrodolfox

cptrodolfox Nov 12, 2019

Author Contributor

One question though, would you mind explaining me I little bit of the pipeline used to update Hackage package set, or pointing me to any doc. Thanks.

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 12, 2019

Member

@cptrodolfox I don't think there is any good documentation, but there is a video explaining it somewhat:

https://discourse.nixos.org/t/video-tutorial-how-to-re-generate-the-haskell-package-set-for-nix/4074

Also, peti does this weekly live on twitch. If you look at past broadcasts, you can watch him do it:

https://www.twitch.tv/peti343/videos

This comment has been minimized.

Copy link
@cptrodolfox

cptrodolfox Nov 12, 2019

Author Contributor

Thanks, I already watch the videos, will see the twitch though.

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 12, 2019

I observed that a new package is available in package-packages (dotenv_0_8_0_4).

I think when a package comes from the LTS package set, hackage2nix is smart enough to also pull in the latest version of a package from hackage and make it available.

Co-Authored-By: Dennis Gosnell <cdep.illabout@gmail.com>
@@ -275,6 +275,7 @@ self: super: {
dlist = dontCheck super.dlist;
docopt = dontCheck super.docopt; # http://hydra.cryp.to/build/499172/log/raw
dom-selector = dontCheck super.dom-selector; # http://hydra.cryp.to/build/497670/log/raw
dotenv = dontCheck super.dotenv;

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 12, 2019

Member

This should be fixed with a more recent version of dotenv, right? Could you add a comment here stating that, so that the nixpkgs maintainers know when we can remove this line?

This comment has been minimized.

Copy link
@cptrodolfox

cptrodolfox Nov 12, 2019

Author Contributor

That comment is ok?

This comment has been minimized.

Copy link
@cdepillabout

cdepillabout Nov 12, 2019

Member

Thanks, very easy to understand!

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 12, 2019

@cptrodolfox Sorry for going back and forth with you so much on this.

Looking good now, so I will merge when CI finishes.

@cptrodolfox

This comment has been minimized.

Copy link
Contributor Author

cptrodolfox commented Nov 12, 2019

Don't worry about that, it is a learning experience for me. I have been a user of NixOS for at least two years so I wanted to learn more about nixpkgs and how to contribute to it.

@cdepillabout

This comment has been minimized.

Copy link
Member

cdepillabout commented Nov 12, 2019

I have been a user of NixOS for at least two years so I wanted to learn more about nixpkgs and how to contribute to it.

Well thanks a lot for taking the time to contribute! We really appreciate you working with us on fixing stuff like this :-)

@cdepillabout cdepillabout merged commit c8628e8 into NixOS:haskell-updates Nov 12, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.