Reduce AsciiDoctor closure size #77149
Merged
+24
−6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@GrahamcOfBorg build asciidoctor |
ofborg
bot
added
6.topic: ruby
10.rebuild-darwin-stdenv
bjornfor
reviewed
Jan 7, 2020
|
<3 <3 <3 |
|
I think the defaultGemConfig changes could be merged straight to master. removing the reference to CC seems a bit risky but worth a try. |
|
in fact, probably changing libxml2 is what creates the biggest rebuild |
|
I suspect there will be packages that relied on the libxml2 Python
bindings being propagated, but don't know how I'd identify them. The
libxml2 change is too big for nix-review to handle. Any ideas?
|
|
|
Can you make this PR without libxml2? Potentially that PR could merged straight into master and then send the libxml2 changes to staging. Another solution would be to add a withPython parameter to libxml2 and then override the version without the python packages just for ruby and its dependencies. |
I'm not sure why this was disabled, but it looks like a pretty harmless way to bring down closure size and remove references to compilers and stuff.
This makes RbConfig["CC"] return an invalid path, but I hope nothing is depending on that anyway...
ext/ isn't needed once the extensions have been built, contains references to a bunch of huge dependencies, and contains megabytes of tests.
(Except on JRuby, where these are presumably important.)
|
Can you make this PR without libxml2? Potentially that PR could merged
straight into master and then send the libxml2 changes to staging.
Done.
|
ofborg
bot
added
10.rebuild-linux: 1001-2500
and removed
10.rebuild-darwin: 501+
10.rebuild-darwin: 5001+
10.rebuild-linux: 5001+
labels
|
Results of 8 packages failed to build:245 packages built:Seems that build failureDidn't try to build other packages, as it would take a long time. |
|
rubyPackages_2_7.gtk2 seems to be broken on master as well. I remember seeing another PR that was fixing this. |
10 tasks
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
@zimbatm, It seems this broke stuff, I now get when I use Can I get the JARs from somewhere else now? |
|
@zimbatm, It seems this broke stuff, I now get
```
Error: Could not find or load main class org.asciidoctor.diagram.CommandServer
asciidoctor: ERROR: index.adoc: line 70: Failed to generate image: end of file reached
```
when I use `-r asciidoctor-diagram` and have plantuml diagrams, and
the error message seems to relate to the removal of the JARs.
Can I get the JARs from somewhere else now?
Are you using JRuby?
|
|
I don't think so, just calling |
|
I've taken a closer look and it appears that the gems are indeed
required for asciidoctor-diagram, on all Rubies, because it runs a Java
process.
So for now we should revert 1ac11cc.
Then, we should look into making all the AsciiDoctor plugins optional,
so the large size of asciidoctor-diagram doesn't affect people who don't
need to use it.
|
|
This is how you can test it |
zimbatm
added a commit
that referenced
this pull request
Mar 4, 2020
This reverts commit 1ac11cc. asciidoctor-diagram starts Java processes, so the JARs are necessary on all platforms. See #77149 (comment).
zimbatm
added a commit
that referenced
this pull request
Mar 4, 2020
This reverts commit 1ac11cc. asciidoctor-diagram starts Java processes, so the JARs are necessary on all platforms. See #77149 (comment). (cherry picked from commit 89a0945)
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
In the documentation format wars, a frequent criticism of AsciiDoctor has been its use closure size. On master, it’s currently 968.1M. With all these changes, it’s 125M. At this point I think we’ve hit the point of diminishing returns — it’s possible to shave off a few more megabytes, but it’s probably not worth it.
Some of these changes are a little risky. I’d like to test with nix-review that nothing that builds before this change fails to build after it. Enough things depend on AsciiDoctor that I think this provides reasonable test coverage. I haven’t done this yet.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @