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

Add override for the vte system library #368

Merged
merged 3 commits into from Aug 26, 2018

Conversation

cdepillabout
Copy link
Member

Add an override to the vte system library so that gnome3.vte is used instead of gnome2.vte.
This is similar to how gtk and gtksourceview work.

This is necessary so that Haskell packages that depend on VTE get setup to use VTE from Gnome3 instead of Gnome2.

This was suggested in NixOS/nixpkgs#44529 (comment).

An overview of why this is needed for the gi-vte package can be found here.

Ideally I'd like to get this PR in to cabal2nix in order to get gi-vte working in time for the 18.09 release.

Add an override to the vte system library to instead use `gnome3.vte`.
This is similar to how gtk and gtksourceview work.

This was suggested in
NixOS/nixpkgs#44529 (comment).
@@ -139,6 +139,7 @@ fromPackageDescription haskellResolver nixpkgsResolver missingDeps flags Package
| i == "gtk2" = binding # (i, path # ["pkgs","gnome2","gtk"])
| i == "gtk3" = binding # (i, path # ["pkgs","gnome3","gtk"])
| i == "gtksourceview3" = binding # (i, path # ["pkgs","gnome3","gtksourceview"])
| i == "vte" = binding # (i, path # ["pkgs","gnome3","vte"])
Copy link
Member

@peti peti Aug 9, 2018

Choose a reason for hiding this comment

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

This change will break all packages that assume vte to come from gnome2. Note that, unlike the examples above it, this entry has no "3" baked into its name and is therefore ambiguous.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch @peti. I just created commit 5fab8e6 that I think will fix this. It specifically checked for the vte-2.91 library (instead of just any version of vte).

You can see that the gi-vte Haskell package is specifically depending on vte-2.91 from Gnome 3:

http://hackage.haskell.org/package/gi-vte-2.91.18/src/gi-vte.cabal

However, the older vte Haskell package is just depending on the vte library from Gnome 2:

https://github.com/gtk2hs/vte/blob/edd23698501af524610153121eff15a03d9c41c3/vte.cabal-renamed#L72

With commit 5fab8e6, both the gi-vte and vte Haskell packages should get created correctly by hackage2nix. gi-vte should be setup to use gnome3.vte, while vte continues to use gnome2.vte.

How does this look?

@cdepillabout
Copy link
Member Author

Hi @peti, I'm wondering if you had a chance to take a look at this PR again?

I fixed your points from #368 (comment), so hopefully it should be ready to merge!

It'd be great if this could get in by NixOS 18.09. I have a package (termonad) that I would like to be available in 18.09, and it is depending on this gi-vte library.

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

This change looks very fragile. That lookup is going to break every time a new version of vte comes out. I doubt that adding this code to cabal2nix will save us a lot of time in the future. It looks more like it's going to cause weird problems that will be hard to debug, i.e. when haskell-packages.nix is suddenly generated differently for no obvious reason because vte-2.91.1 was released.

@jtojnar
Copy link
Contributor

jtojnar commented Aug 17, 2018

vte-2.91 is pkg-config name. If it gets bumped, we will package it as a new expression (vte4).

@peti
Copy link
Member

peti commented Aug 17, 2018

It is categorically impossible to release a bugfix for vte-2.91 that has a higher version number yet still goes into the gnome3 hierarchy?

@jtojnar
Copy link
Contributor

jtojnar commented Aug 17, 2018

If the .pc name file was changed before the release GNOME 4, we would create a new package gnome3.vte4, or more likely move all VTE to top-level as has been done with GtkSourceView:

https://github.com/NixOS/nixpkgs/commits/a9dbcdb6269a68fdce7b229f9ce98f8b241cd60b/pkgs/development/libraries/gtksourceview

Either way, an attribute should have stable .pc name.

@cdepillabout
Copy link
Member Author

@peti

The situation for VTE is a little confusing. To rehash what @jtojnar said, vte-2.91 is the name of the pkgconf file and can be thought of as the full package name (as opposed to packagename-version). vte-2.91 is the VTE package that is currently being used in Gnome 3.

vte-2.91 has a version as well. For example, in master for nixpkgs, it is currently 0.52.2. So if you were to write packagename-version for VTE, it would look like vte-2.91-0.52.2.

There is also an older version of VTE that works with Gnome 2. I believe its pkgconfig file is just called vte, so if you were to write packagename-version for it, it would look like vte-0.28.2.

This PR makes it so that both VTE from Gnome 3 and Gnome 2 work correctly when used with cabal2nix (and hackage2nix).

@cdepillabout
Copy link
Member Author

Hi @peti, any chance to take a look at this? I believe this PR should be good to be merged as-is.

@@ -147,6 +147,7 @@ libNixName "taglib_c" = return "taglib"
libNixName "tensorflow" = return "libtensorflow"
libNixName "udev" = return "systemd";
libNixName "uuid" = return "libossp_uuid";
libNixName "vte-2.91" = return "vte-2.91"
Copy link
Member

Choose a reason for hiding this comment

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

This will result in a Nix expression with invalid syntax since a . is not allowed in a Nix identifier:

$ nix eval '(let vte-2.91 = "foo"; in vte-2.91)'
error: syntax error, unexpected FLOAT, expecting '.' or '=', at (string):1:11

Copy link
Member

Choose a reason for hiding this comment

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

After inspecting nixpkgs I think this should be:

libNixName "vte-2.91" = return "vte_290"

Then you need to use "vte_290" in resolveInNixpkgs as well.

Copy link
Member

Choose a reason for hiding this comment

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

Please ignore my previous comment since you require vte-2.91 and my suggestion will give you vte-2.90 which will be too old presumably.

Copy link
Member

Choose a reason for hiding this comment

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

Introducing a fake name like "vte_291" might work. So:

libNixName "vte-2.91" = return "vte_291"
    resolveInNixpkgs i
      ...
      | i == "vte_291" = binding # (i, path # ["pkgs","gnome3","vte"])

I'm not sure this is a hack but I think it might work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would moving vte to nixpkgs top-level help? I think we will want to do that eventually anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@basvandijk I was under the impression that it would be okay because I was overriding it in both resolveInNixPkgs and libNixName.

But I'll go ahead and change it just in case!

Copy link
Member

Choose a reason for hiding this comment

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

FYI, you can have dots in attribute names if they're quoted: { "foo.bar" = null; }

This fake name is just used to match within the `resolveInNixpkgs`
function.
@cdepillabout
Copy link
Member Author

@basvandijk @jtojnar @shlevy Thanks for reviewing this PR.

@basvandijk I've made a change like you suggested in commit 5955759.

The fake name vte_291 has been introduced. It is matched on in the resolveInNixpkgs function.

@basvandijk basvandijk merged commit ff2693c into NixOS:master Aug 26, 2018
@basvandijk
Copy link
Member

@cdepillabout thank you!

@cdepillabout
Copy link
Member Author

@basvandijk Thanks for reviewing and merging it in!

@cdepillabout cdepillabout deleted the add-vte-to-resolveInNixpkgs branch August 26, 2018 12:45
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.

None yet

5 participants