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

dotenv: fix broken package #73256

Merged
merged 6 commits into from Nov 12, 2019
Merged

dotenv: fix broken package #73256

merged 6 commits into from Nov 12, 2019

Conversation

cptrodolfox
Copy link
Contributor

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
Copy link
Member

cdepillabout commented Nov 11, 2019 •
edited

@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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

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

@cdepillabout
Copy link
Member

@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
Copy link
Contributor Author

cptrodolfox commented Nov 12, 2019 •
edited

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.

@cptrodolfox cptrodolfox changed the title dotenv: fix broken package and updated megaparsec dotenv: fix broken package Nov 12, 2019
cdepillabout
cdepillabout reviewed Nov 12, 2019
View reviewed changes
pkgs/development/haskell-modules/configuration-nix.nix Outdated


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

Choose a reason for hiding this comment

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

@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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just moved the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@cdepillabout
Copy link
Member

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.

@cptrodolfox @cdepillabout
Update pkgs/development/haskell-modules/configuration-nix.nix
3376721 
Co-Authored-By: Dennis Gosnell <cdep.illabout@gmail.com>
cdepillabout
cdepillabout reviewed Nov 12, 2019
View reviewed changes
pkgs/development/haskell-modules/configuration-common.nix Outdated
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is ok?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, very easy to understand!

@cdepillabout
Copy link
Member

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

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

@cptrodolfox
Copy link
Contributor Author

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
Copy link
Member

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
@cdepillabout cdepillabout mentioned this pull request Nov 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdepillabout cdepillabout cdepillabout left review comments

@basvandijk basvandijk Awaiting requested review from basvandijk

Assignees
No one assigned
Labels
6.topic: haskell 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cptrodolfox @cdepillabout