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

[staging] asciidoc: 8.6.9 -> 9.0.4 #102398

Merged
merged 1 commit into from Jan 21, 2021

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Nov 1, 2020

Main motivation was to port it to Python3 which is due to #101964.
So:

  • Switch from asciidoc to asciidoc-py3
  • Switch from Python 2 to Python 3
  • This needs autoreconfHook now
  • We also need to do some patching in a2x.py
  • Switch to patchShebangs (more readable)
  • Only input w3m if we actually build with it
Motivation for this change

#101964

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.

@dasJ
Copy link
Member Author

dasJ commented Nov 2, 2020

Sooooo I successfully built asciidoc and asciidoc-full. I will try asciidoc-full-with-plugins tomorrow because I'm going to bed now and it doesn't seem to be done any time soon

@dasJ
Copy link
Member Author

dasJ commented Nov 2, 2020

Alright, asciidoc-full-with-plugins builds as well

chmod +x "$n"
done
'' else ''
sed -e "s|^ENV =.*|ENV = dict(XML_CATALOG_FILES='${docbook_xml_dtd_45}/xml/dtd/docbook/catalog.xml ${docbook_xsl_ns}/xml/xsl/docbook/catalog.xml ${docbook_xsl}/xml/xsl/docbook/catalog.xml')|" \
Copy link
Contributor

Choose a reason for hiding this comment

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

this first bit here shouldn't be needed. if the docbook dependencies are added to nativeBuildInputs you will have the setup hook findXMLCatalogs which will set this appropriately.

Copy link
Member

Choose a reason for hiding this comment

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

@dasJ does this need to be addressed still? I don't have enough context to judge.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably the same as my other comment

Comment on lines +258 to +259
sed -e "s|^ENV =.*|ENV = dict(XML_CATALOG_FILES='${docbook_xml_dtd_45}/xml/dtd/docbook/catalog.xml ${docbook_xsl_ns}/xml/xsl/docbook/catalog.xml ${docbook_xsl}/xml/xsl/docbook/catalog.xml')|" \
-e "s|^XSLTPROC =.*|XSLTPROC = '${libxslt.bin}/bin/xsltproc'|" \
-e "s|^XMLLINT =.*|XMLLINT = '${libxml2.bin}/bin/xmllint'|" \
Copy link
Contributor

Choose a reason for hiding this comment

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

From looking at the a2x.py it just wants these in PATH. So I think also, these programs being in nativeBuildInputs should be enough?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@zimbatm #102398 (comment)

While I don't really know enough about asciidoc to judge this myself, I think we cannot just put them into nativeBuildInputs since that would require putting them into the nativeBuildInputs of every build that uses a2x

@worldofpeace
Copy link
Contributor

Oh wait, a2x is a part of asciidoc. So that is needed.

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 11, 2020

In that case, it would really be better to do a patch on a2x fix-paths.patch with substituteAll. See nixpkgs for examples (mostly gnome packaging or that actual file name mentioned)

Edit: How exactly does a2x work if we configure it with

ENV = dict(XML_CATALOG_FILES='${docbook_xml_dtd_45}/xml/dtd/docbook/catalog.xml ${docbook_xsl_ns}/xml/xsl/docbook/catalog.xml ${docbook_xsl}/xml/xsl/docbook/catalog.xml')

I see the description converts Asciidoc text files to other file formats with a switch

 -f FORMAT, --format=FORMAT
                        chunked, epub, htmlhelp, manpage, pdf, text, xhtml,
                        dvi, ps, tex, docbook

I would then think, does it matter what version of the docbook catalogs are used? Conversion between formats and being to distinguish the docbook version is probably needed (not familiar with the tool but I can imagine it being used to convert documentation in other projects builds even).

@FRidh FRidh added this to the 21.03 milestone Nov 19, 2020
@dasJ
Copy link
Member Author

dasJ commented Nov 21, 2020

Yeah, I can do the stuff with the patch.

About your question with the catalog and stuff: I don't know.
When building with enableStandardFeatures, these patches are applied as well and they also seem to be required when building without standard features now.
Later a2x executions (by downstream builds) failed, so inserted the paths.

@dasJ
Copy link
Member Author

dasJ commented Nov 21, 2020

Yeah, this is the error when not setting the catalog files:

  python3 a2x.py -f manpage doc/asciidoc.1.txt
  a2x: ERROR: "/nix/store/l75cxsdxrzkq5s4h4si9rql0xgjf0bax-libxml2-2.9.10-bin/bin/xmllint" --nonet --noout --valid "/build/source/doc/asciidoc.1.xml" returned non-zero exit status 4

@dasJ
Copy link
Member Author

dasJ commented Nov 21, 2020

@worldofpeace Still want me to switch to a patch and substituteAll?

@raboof
Copy link
Member

raboof commented Dec 13, 2020

Result of nixpkgs-review pr 102398 run on x86_64-linux 1

3 packages built:
  • btrfs-progs
  • ninja
  • zeromq

@ajs124 ajs124 added this to Needs review in Python 2 deprecation Dec 19, 2020
@raboof
Copy link
Member

raboof commented Dec 27, 2020

I'd love to see this advance, anything I can do to help?

Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

Pure code review looks good to me

pkgs/tools/typesetting/asciidoc/default.nix Outdated Show resolved Hide resolved
pkgs/tools/typesetting/asciidoc/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from bjornfor December 27, 2020 21:41
@dasJ dasJ force-pushed the upd/asciidoc2 branch 2 times, most recently from e5312ee to dacdb88 Compare January 4, 2021 08:06
@ofborg ofborg bot requested a review from lovek323 January 4, 2021 08:16
@ThibautMarty
Copy link
Member

Is it possible to fix the asciidoc package when used in a cross-compiled package? If I'm not wrong it's currently broken.
For instance, ccache has asciidoc in its nativeBuildInputs and I get (with this patch applied):

nix-repl> builtins.filter (p: lib.getName p == "asciidoc") ccache.nativeBuildInputs
[ «derivation /nix/store/i1z08c0gbjm9217h3kjd0jsb0z4z1qwg-asciidoc-9.0.4.drv» ]

nix-repl> builtins.filter (p: lib.getName p == "asciidoc") pkgsCross.aarch64-multiplatform.ccache.nativeBuildInputs
[ «derivation /nix/store/l9sysn6s2r9wbyh58sa11ds2dpygw8n1-asciidoc-9.0.4-aarch64-unknown-linux-gnu.drv» ]

I'd expect to get the same derivation for asciidoc in both cases. In particular, in the latter derivation the python scripts have aarch64 python shebangs (In fact the build fails somewhere on several nixpkgs commits I tried: so I'm not 100% sure but I remembered this).

I suppose handling dependencies without going through the *Inputs scheme (e.g., sed -e "s|dot|${graphviz}/bin/dot|g") is expected to break cross compilation? And/or maybe asciidoc should not be in nativeBuildInputs but in an other *Inputs (it is some kind of compiler)?

@raboof
Copy link
Member

raboof commented Jan 18, 2021

(needs a rebase, it seems)

Main motivation was to port it to Python3 which is due to NixOS#101964.
So:

- Switch from asciidoc to asciidoc-py3
- Switch from Python 2 to Python 3
- This needs autoreconfHook now
- We also need to do some patching in a2x.py
- Switch to patchShebangs (more readable)
- Only input w3m if we actually build with it
@dasJ
Copy link
Member Author

dasJ commented Jan 18, 2021

@raboof Rebased

@ThibautMarty good point, sadly I don't really know a lot about cross compiling. Can anyone help out here? Or do you think it might be possible that someone else (with more knowledge in this field) fixes this in a later PR?

@zimbatm
Copy link
Member

zimbatm commented Jan 18, 2021

@dasJ there are two left-over comments in the review. Do you mind taking a look?

Cross is not officially supported so it can be fixed async.

@dasJ
Copy link
Member Author

dasJ commented Jan 19, 2021

I cannot really judge #102398 (comment) since I actually don't really know how it all plays together. I chose the sed way because this package seems to do that extensively, and I wanted to keep it consistent while not having to refactor the whole package.

@zimbatm
Copy link
Member

zimbatm commented Jan 21, 2021

Let's merge this and clean up afterwards if any breakage happens.

@zimbatm zimbatm merged commit e48b0ed into NixOS:staging Jan 21, 2021
Python 2 deprecation automation moved this from Needs review to Done Jan 21, 2021
@zimbatm
Copy link
Member

zimbatm commented Jan 21, 2021

@dasJ please keep an eye on the staging channel on hydra :)

@dasJ dasJ deleted the upd/asciidoc2 branch January 21, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants