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

Jump to bottom

Update noweb (and icon-lang) #53734

Merged
merged 4 commits into from Aug 13, 2019
Merged

Update noweb (and icon-lang) #53734

merged 4 commits into from Aug 13, 2019

Conversation

yurrriq
Copy link
Member

@yurrriq yurrriq commented Jan 10, 2019 •
edited

Motivation for this change

As per src/INSTALL:

Crucial parts of noweb are written in both Icon and Awk. You can use
either Icon or Awk, but Icon is better

Most notable (to me, anyway) are the performance increase and the autodefs.

Things done

  • Update icon-lang

    • Add Darwin support
      • set name appropriately in configurePhase
      • meta.platforms: linux -> linux ++ darwin
    • Add me as maintainer
    • Support nongraphical build (via withGraphics=false)
      • Use the right configure target
      • Don't depend on libX{11,t}

  • Update noweb

    • fetchurl -> fetchFromGitHub
    • Update version: 2.11b -> 2.12
    • Configure for use as TeX Live package
    • Tweak quotes and whitespace for safety/consistency/readability
    • Flesh out meta
    • Install noweb-mode to $out/share/emacs/site-lisp
    • Provide noweb_awk and noweb_icon, where noweb is the latter

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)

  • 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 nox --run "nox-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)

  • Assured whether relevant documentation is up to date

  • Fits CONTRIBUTING.md.

@yurrriq yurrriq force-pushed the update/noweb branch 5 times, most recently from 7e37326 to 97479ca Compare January 15, 2019 00:33
@yurrriq yurrriq changed the title WIP: Update noweb (and icon-lang) Update noweb (and icon-lang) Jan 15, 2019
@yurrriq
Copy link
Member Author

yurrriq commented Jan 15, 2019 •
edited

I'd love some more eyes on this, but it seems to work great for me so far on NixOS as well as macOS 10.14.2. I pulled out the Lisp autodefs patch, because of an apparent hen and egg issue.

yurrriq added a commit to yurrriq/nur-packages that referenced this pull request Jan 15, 2019
@yurrriq
Update noweb and icon-lang
3d6752d 
NixOS/nixpkgs#53734
@yurrriq yurrriq changed the title Update noweb (and icon-lang) WIP: Update noweb (and icon-lang) Jan 15, 2019
@yurrriq
Copy link
Member Author

yurrriq commented Jan 15, 2019 •
edited

Ok, so there seems to be some problem with icon-lang, maybe. When I clone the repos and build icon and noweb manually. All is well. Any advice?

Specifically, totex sometimes fails with:

error in startup code
icode file too large
error in startup code
icode file too large

@yurrriq yurrriq changed the title WIP: Update noweb (and icon-lang) Update noweb (and icon-lang) Apr 19, 2019
@yurrriq
Copy link
Member Author

yurrriq commented Apr 19, 2019 •
edited

Got it sorted. Turns out substituteInPlace is destructive to Icon scripts. As a workaround, I added a call to grep nawk, so we only call substituteInPlace on appropriate files.

@yurrriq
Copy link
Member Author

yurrriq commented Apr 19, 2019 •
edited

@ryantm, will you please remove the label 2.status: work-in-progress? This is ready to review/merge now. :)

Edit: Thanks!

@yurrriq
Copy link
Member Author

yurrriq commented Apr 19, 2019

@GrahamcOfBorg build icon-lang
@GrahamcOfBorg build noweb

@yurrriq
Copy link
Member Author

yurrriq commented Apr 19, 2019

What do you think, @vrthra?

@yurrriq yurrriq force-pushed the update/noweb branch 3 times, most recently from 17a5b67 to 90dba9d Compare April 20, 2019 08:26
@yurrriq
Copy link
Member Author

yurrriq commented Apr 20, 2019

@GrahamcOfBorg build noweb_awk
@GrahamcOfBorg build noweb_icon

@yurrriq yurrriq force-pushed the update/noweb branch 2 times, most recently from 5f56351 to 9450feb Compare April 25, 2019 01:11
@yurrriq
Copy link
Member Author

yurrriq commented Apr 25, 2019

@GrahamcOfBorg build noweb_awk
@GrahamcOfBorg build noweb_icon

@yurrriq
Copy link
Member Author

yurrriq commented May 10, 2019

Is there anything I can do to push this along? I've been using it for a while now and am quite satisfied. I'd like to add some sort of plugin system, using the lisp autodefs script as the first one, but I won't get to that for a while, and it seems like it ought to be a separate PR anyway.

@yurrriq
Copy link
Member Author

yurrriq commented Jun 7, 2019

Can someone merge this?

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/3

aanderse
aanderse reviewed Aug 7, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Please address comment by @lilyball.

Without a domain expert it makes this more challenging to merge which is why I expect there has been such a delay up to this point. Unfortunate that @vrthra is unable to provide feedback. Thank you for adding yourself to the maintainers list.

After all comments have been addressed we will wait a few days to see if we get any domain experts from discourse and then I suggest merge as overall this change looks reasonable to me.

@ryantm and @lilyball does this sound acceptable to you?

pkgs/development/tools/literate-programming/noweb/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/literate-programming/noweb/default.nix Show resolved Hide resolved
@yurrriq
Copy link
Member Author

yurrriq commented Aug 7, 2019 •
edited

For the record, I've been using this derivation for a year or so at this point, without issue, on macOS and NixOS.

@yurrriq
noweb: 2.11b -> 2.12
67aaa70 
- fetchurl -> fetchFromGitHub
- Support building with icon (useIcon, default: false)
- Support darwin
- Configure for texlive
- Flesh out meta
- Build man pages
- Pass through tex package
@aanderse
Copy link
Member

@GrahamcOfBorg build noweb_awk noweb_icon

@yurrriq
Copy link
Member Author

yurrriq commented Aug 13, 2019

Looks like we need gcc, at least on Darwin?

@lilyball
Copy link
Member

Better yet, just patch the Makefile to use clang instead. The Makefile even has a comment indicating that you should adjust it to use your preferred ANSI C compiler.

@lilyball
Copy link
Member

Or define CC in the command-line flags given to make, which will override the variable definition in the Makefile.

@yurrriq
Copy link
Member Author

yurrriq commented Aug 13, 2019

Can someone please trigger the darwin build?

@Mic92
Copy link
Member

Mic92 commented Aug 13, 2019

@GrahamcOfBorg build noweb_awk noweb_icon

@Mic92 Mic92 merged commit 108c57b into NixOS:master Aug 13, 2019
@yurrriq
Copy link
Member Author

yurrriq commented Aug 13, 2019

🎉 thanks for the triage and merging!

@yurrriq yurrriq deleted the update/noweb branch August 13, 2019 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilyball lilyball lilyball left review comments

@ryantm ryantm ryantm left review comments

@nrnrnr nrnrnr nrnrnr left review comments

@Mic92 Mic92 Mic92 left review comments

@aanderse aanderse aanderse left review comments

@vrthra vrthra Awaiting requested review from vrthra

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@yurrriq @nixos-discourse @aanderse @lilyball @Mic92 @ryantm @nrnrnr @GrahamcOfBorg