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

[haskell-updates] Fix reflex, reflex-dom and dependencies. #90260

Merged
merged 1 commit into from
Jun 19, 2020
Merged

[haskell-updates] Fix reflex, reflex-dom and dependencies. #90260

merged 1 commit into from
Jun 19, 2020

Conversation

maralorn
Copy link
Member

@maralorn maralorn commented Jun 13, 2020 •
edited
Loading

Motivation for this change

Would be really nice to be able to build reflex packages with nixpkgs without needing to rely on https://github.com/reflex-frp/reflex-platform in all cases.

Todo List
  • Discuss adding jailbreakConditional conditional dependencies to cabal-jailbreak
  • Create upstream issues about version bounds
  • Make PR for patch work with newer dependent-sum
  • Create upstream issues about old test dependencies
  • Figure out why reflex-dom-core remeins marked as broken
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)
  • 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.

@maralorn
Copy link
Member Author

I need help on this one. @peti I have run hackage2nix and reflex-dom-core is not being mentioned at all in configuration-hackage2nix.yaml. But still there is a broken flag generated in the hackage-packages.nix expression for reflex-dom-core. Any hints?

@maralorn maralorn force-pushed the fix-reflex-dom branch 2 times, most recently from 1942696 to 5d0da0e Compare June 14, 2020 00:48
@peti
Copy link
Member

peti commented Jun 17, 2020

reflex-dom-core is considered broken, because it has a dependency on the non-existent Haskell package chrome-test-utils. Note the explicit null assignment that's generated at the end of the expression.

@maralorn
Copy link
Member Author

Ah, thank you @peti. Well that wont be a problem, if we disable the tests.

@maralorn maralorn marked this pull request as ready for review June 19, 2020 00:31
@maralorn maralorn requested a review from cdepillabout as a code owner June 19, 2020 00:31
@maralorn
Copy link
Member Author

btw. can I do something to get a Haskell label on my PR?

@maralorn
Copy link
Member Author

maralorn commented Jun 19, 2020 •
edited
Loading

I don‘t feel like I have anything to do with the build error @GrahamcOfBorg is complaining about.
The build works just fine on my machine.
EDIT: Yeah, the same error is reported on the haskell-updates branch.

@maralorn maralorn changed the title [haskellUpdates] Fix reflex, reflex-dom and dependencies. [haskell-updates] Fix reflex, reflex-dom and dependencies. Jun 19, 2020
@cdepillabout
Copy link
Member

@maralorn

can I do something to get a Haskell label on my PR?

I think ofborg is supposed to do it automatically. I'm not sure what is happening here.

the build error @GrahamcOfBorg is complaining about

I actually just figured out how to reproduce this locally, and I wrote a short blog post about it:
https://functor.tokyo/blog/2020-06-02-evaluating-nixpkgs

I think you're right that these errors aren't related to this PR. However, they are things that prevent PRs like #90032 from being merged in. It is mostly @peti who fixes these up, but if you were willing to send a PR fixing some of these evaluation errors, @peti and I would really appreciate it. (It would make @peti's job easier when he goes and merges haskell-updates into master.)

cdepillabout
cdepillabout reviewed Jun 19, 2020
View reviewed changes
pkgs/development/haskell-modules/configuration-common.nix Outdated
Comment on lines 1513 to 1518
postPatch = (drv.postPatch or "") + ''
substituteInPlace reflex.cabal --replace "haskell-src-exts >=1.16 && <1.23" "haskell-src-exts -any"
substituteInPlace reflex.cabal --replace "these >=1 && <1.1" "these -any"
substituteInPlace reflex.cabal --replace "dependent-sum >=0.6 && <0.7" "dependent-sum -any"
'';
})));
Copy link
Member

Choose a reason for hiding this comment

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

In general, I think @peti prefers these types of things to be done in a fetchpatch, so it is obvious when they are no longer needed (when the patch stops applying cleanly).

There should be a bunch of examples in this file of using fetchpatch to pull down patches from a given PR.

(Same for the reflex-dom-core thing below.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is fetchpatch on an upstream PR in general preferable to e.g. jailbreaking?

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 have decided that yes.

Copy link
Member

@cdepillabout cdepillabout Jun 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is fetchpatch on an upstream PR in general preferable to e.g. jailbreaking?

I think there are two different things going on here:

  1. Is it preferable to use fetchpatch over substituteInPlace for fixing versioning problems?

    Peti has said that he prefers fetchpatch in these situations so we know exactly when the patch is no longer valid and can be removed.

  2. Is it preferable to use fetchpatch over doJailbreak?

    This is less straightforward, and more depends on the situation. In general, fetchpatch is preferable, but there are lots of Haskell libraries that don't particularly follow the latest LTS (instead they follow an older LTS), so sending a PR to them is just annoying. They don't particularly appreciate it, because they don't intend to chase the latest LTS. In these cases, doJailbreak can be easier.

cdepillabout
cdepillabout reviewed Jun 19, 2020
View reviewed changes
pkgs/development/haskell-modules/configuration-common.nix Outdated
Comment on lines 1521 to 1522
# Tests disabled and broken overrided needed because of missing lib chrome-test-utils.
reflex-dom-core = unmarkBroken (dontCheck (doJailbreak (overrideCabal (super.reflex-dom-core.override {
Copy link
Member

Choose a reason for hiding this comment

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

Another couple possibilities:

  • Bug upstream to release chrome-test-utils to Hackage. (It is frustrating when people release libraries to Hackage that have dependencies that aren't on Hackage. It is also somewhat frustrating that Hackage allows this.)

  • Send a PR to cabal2nix teaching it about this special case for reflex-dom-core, where tests shouldn't be run, and it shouldn't be marked broken. (Although I don't know if this will work well, since there could be multiple reasons for it to be marked broken.)

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 am bugging upstream about it. Do you consider this a merge blocker in between?
Special casing this in cabal2nix sounds very ugly to me.

cdepillabout
cdepillabout reviewed Jun 19, 2020
View reviewed changes
pkgs/development/haskell-modules/hackage-packages.nix Outdated
Comment on lines 69850 to 69851
license = "unknown";
hydraPlatforms = stdenv.lib.platforms.none;
Copy link
Member

Choose a reason for hiding this comment

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

This seems unfortunate. Maybe something needs to be fixed upstream??

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, huh. I didn‘t notice this.

Which upstream do you mean?

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 am actually not sure if this is normal for extra-packages?

Anyways I got rid of that dependency, so it doesn’t matter for this PR anymore.

@maralorn maralorn mentioned this pull request Jun 19, 2020
Open
@maralorn maralorn force-pushed the fix-reflex-dom branch 4 times, most recently from f817ae9 to 881c632 Compare June 19, 2020 15:54
@peti peti merged this pull request into NixOS:haskell-updates Jun 19, 2020
@maralorn
Copy link
Member Author

@peti Upstream has agreed, that the missing dependency on hackage is bad. (reflex-frp/reflex-dom#392 (comment)) So I hope we will see a fix there at some point.

@maralorn maralorn deleted the fix-reflex-dom branch June 21, 2020 14:35
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

Assignees
No one assigned
Labels
6.topic: haskell
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maralorn @peti @cdepillabout