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

Cygwin hooks for stdenvNative and cygwin fixes for various packages #6101

Closed
wants to merge 27 commits into from

Conversation

chaoflow
Copy link
Member

@chaoflow chaoflow commented Feb 2, 2015

Todo:

  • rebased onto current master
  • publish technical report detailing this work
  • Verify perl 5.20 builds with windows, it was perl 5.16 when we tested the last time.
  • proof-read rebased diff that non-cygwin stdenv is untouched
  • travis error(s) - I don't understand the current error

@peti
Copy link
Member

peti commented Feb 2, 2015

Could you please rebase this branch relative to the current master and force-push the new version?

@garbas
Copy link
Member

garbas commented Feb 2, 2015

@peti i'll do it during this week

@lucabrunox
Copy link
Contributor

So much to review. Maybe this can be splitted in multiple commits? Don't know.

@@ -0,0 +1,1710 @@
diff -ur w3m-0.5.3/config.guess new/w3m-0.5.3/config.guess
Copy link
Member

Choose a reason for hiding this comment

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

Was this just generated by autoconf? Might be simpler (and easier to review) to just call autoconf as part of the build, if that doesn't break stuff

@copumpkin
Copy link
Member

Some of the .patch files in there are huge. Unless you made all the changes yourself (in which case we should probably look at them very carefully), It seems like it would be cleaner to just refer to wherever the new versions of the source files come from.

@@ -255,6 +255,7 @@ stdenv.mkDerivation rec {
'';

preInstall = "mkdir -p $out/etc/vim";
makeFlags = if stdenv.isCygwin then "DESTDIR=/." else null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an underlying issue with make on cygwin or is it just asciidoc that has problems?

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's only asciidoc. Without this "fix" it produces a "//" at the beginning of the path which on cygwin is translated to "" which is the beginning of network SAMBA path.

@chaoflow
Copy link
Member Author

chaoflow commented Feb 3, 2015

@copumpkin: The patches are mostly taken from cygwin tarballs, i.e. there are no direct urls to them.

@garbas
Copy link
Member

garbas commented Feb 4, 2015

@chaoflow @copumpkin what if we create derivation for those patches and extract patches ? it should be possible to refer to patches easier to keep them up to date.

@chaoflow chaoflow force-pushed the cygwin branch 3 times, most recently from 067fb41 to b12991a Compare February 10, 2015 09:05
@@ -8,7 +8,7 @@ let
gitBase = lib.makeOverridable (import ./git) {
inherit fetchurl stdenv curl openssl zlib expat perl python gettext gnugrep
asciidoc xmlto docbook2x docbook_xsl docbook_xml_dtd_45 libxslt cpio tcl
tk makeWrapper subversionClient gzip;
tk makeWrapper subversionClient gzip libiconvOrNull;
Copy link
Member

Choose a reason for hiding this comment

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

Note that libiconvOrNull will probably go away soon: #6166

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 understand #6199 in the sense that libiconv will replace the functionality of libiconvOrNull. Either this is merged first and libiconvOrNull occurrences will be fixed as part of #6199 or we do it the other way round.

chaoflow added a commit that referenced this pull request Feb 14, 2015
chaoflow added a commit that referenced this pull request Feb 14, 2015
Picked from #6101.

(cherry picked from commit e82e14e)
@vcunat
Copy link
Member

vcunat commented Feb 14, 2015

Apart from the "sed-i" issue, non-cygwin impact seems relatively safe, but it's still a mass rebuild, and it will be better to screen this for larger build regressions before merging to staging.

As for the Travis error, it sounds like an escaping issue unrelated to this. We have python27Packages."plone.synchronize-1.0.1", so there are dots inside the attribute name (which is really ugly). The message seems like the quotation is missed (searching for plone attribute).

@vcunat
Copy link
Member

vcunat commented Feb 14, 2015

Patches: as suggested, on your place I'd consider creating a derivation that extracts all the cygwin patches, so it's easier to update (once upon a time); then they could be used like optional stdenv.isCygwin "${cygwinPatches}/pkgconfig-foo.patch".

@copumpkin
Copy link
Member

Is this still alive? It seems exciting!

@garbas
Copy link
Member

garbas commented Apr 15, 2015

@copumpkin still alive. i hope to find time to finish this soon.

@chaoflow chaoflow force-pushed the cygwin branch 7 times, most recently from 8ee83ce to ee365e9 Compare May 26, 2015 18:35
@garbas
Copy link
Member

garbas commented May 28, 2015

"if" was used instead of "stdenv.lib.optional/stdenv.lib.optionals/stdenv.lib.optionalString" to not trigger stdenv rebuild.

@vcunat
Copy link
Member

vcunat commented May 28, 2015

I see, but for the sake of readability... on staging we do a stdenv rebuild almost every week.

@garbas
Copy link
Member

garbas commented May 28, 2015

@vcunat this PR is being here for almost half a year. it is really hard to keep it up to date because of patches changing for every version. i would really like to merged it as is, otherwise we would lose what we have currently working.

@vcunat
Copy link
Member

vcunat commented May 28, 2015

OK, I thought the PR is waiting for you to finish something

i hope to find time to finish this soon.

@garbas
Copy link
Member

garbas commented May 28, 2015

it is an old comment @chaoflow and I rebased it 2 days ago.

@garbas
Copy link
Member

garbas commented May 28, 2015

manually merged here

@garbas garbas closed this May 28, 2015
@vcunat
Copy link
Member

vcunat commented May 28, 2015

I see; github doesn't notify participants when you add or change PR commits.

@chaoflow
Copy link
Member Author

chaoflow commented Sep 8, 2015

Meanwhile, we got around to publish the report: http://ternaris.com/lab/nix-on-windows.html

adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
Picked from NixOS#6101.

(cherry picked from commit e82e14e)
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

10 participants