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

resolves #1258 upgrade to Asciidoctor 2.0.x #1264

Merged

Conversation

mojavelinux
Copy link
Contributor

  • upgrade to Asciidoctor 2.0.x (starting with 2.0.5)
  • switch to fuzzy version match for asciidoctor gem
  • disable sectanchors (since they get removed by the sanitizer anyway)
  • use formal xref macro for interdocument xref in test
  • add test for toggling the document title using showtitle/!showtitle
  • add test to verify toc is generated at top of document when toc attribute is set

- upgrade to Asciidoctor 2.0.x (starting with 2.0.5)
- switch to fuzzy version match for asciidoctor gem
- disable sectanchors (since they get removed by the sanitizer anyway)
- use formal xref macro for interdocument xref in test
- add test for toggling the document title using showtitle/!showtitle
- add test to verify toc is generated at top of document when toc attribute is set
@mojavelinux
Copy link
Contributor Author

@kivikakk Now that Asciidoctor follows semantic versioning, we can safely use a fuzzy match on the patch version. If you'd rather stick with a fixed version, just let me know and I'd be happy to comply.

I also added a few more tests for scenarios I could think of that are relevant for rendering AsciiDoc on GitHub.

@kivikakk
Copy link
Contributor

kivikakk commented Apr 2, 2019

Nice! All passing locally, CI should be green any moment, but I'll prepare a prerelease gem of markup and run the full github.com CI suite with that in place and see if anything falls out.

@kivikakk
Copy link
Contributor

kivikakk commented Apr 2, 2019

Just fired off CI, it's the end of my work day so I'll check back in first thing tomorrow! Should be pretty straightforward. 👍

@mojavelinux
Copy link
Contributor Author

Excellent! Thank you so much for your time and attention.

@kivikakk
Copy link
Contributor

kivikakk commented Apr 2, 2019

Nice! One small test change required on the dotcom end, but looking good. I'll make a new Markup release and start the process to get the change in!

@kivikakk kivikakk merged commit d799e19 into github:master Apr 2, 2019
@mojavelinux mojavelinux deleted the issue-1258-upgrade-to-asciidoctor-2 branch April 2, 2019 23:17
@mojavelinux
Copy link
Contributor Author

Thanks @kivikakk!

@kivikakk
Copy link
Contributor

kivikakk commented Apr 4, 2019

Unfortunately I had to abort the roll-out of this; our workers fail to boot up and throw an ArgumentError with the text couldn't find login name -- expanding `~', from this line: https://github.com/asciidoctor/asciidoctor/blob/v2.0.5/lib/asciidoctor.rb#L188

I'm guessing our workers do not expose a HOME environment variable and that getlogin gave nothing either; see https://github.com/ruby/ruby/blob/53d3fe0643c591a9083e22ccea62e49f451fd450/file.c#L3559.

It's probably going to be difficult to change this state of affairs any time soon. Is it possible to make Asciidoctor not rely on there being a valid user home directory defined?

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Apr 4, 2019

Ah yes. In 1.5.x we had a rescue on that line. I couldn't remember why it was there, so I removed it in 2.0. Now that I know why it was there, I can restore it, document it, and push out a new version (2.0.6). I'll let you know once it's available!

@mojavelinux
Copy link
Contributor Author

Thanks for the feedback!

@kivikakk
Copy link
Contributor

kivikakk commented Apr 4, 2019

Great, thank you! (And indeed, the rescue tracks back to asciidoctor/asciidoctor#896. It's strange this happens as often as it does. My best guess is a sudo policy that clears the environment entirely.)

@mojavelinux
Copy link
Contributor Author

Upstream issue: asciidoctor/asciidoctor#3238

@mojavelinux
Copy link
Contributor Author

It's indeed strange, but surprisingly prevalent. I've seen Docker containers have this same problem, so this fix turns out to be quite important. This is a good catch.

@mojavelinux
Copy link
Contributor Author

@kivikakk
Copy link
Contributor

kivikakk commented Apr 5, 2019

@mojavelinux Cheers, I'll start on the upgrade work! No new Markup release is needed since the Gemfile is only used in development anyway, I'll just bump the Asciidoctor gem in my github PR.

@mojavelinux
Copy link
Contributor Author

Perfect! Thank you so much. If there's anything else you need, just let me know and I'll get it taken care of asap.

@kivikakk
Copy link
Contributor

kivikakk commented Apr 5, 2019

Thank you! Will do.

@kivikakk
Copy link
Contributor

kivikakk commented Apr 5, 2019

➡️ https://github.com/kivikakk/what-version-of-asciidoctor-is-github-on ❗️

Let me know if anything looks off to you.

@mojavelinux
Copy link
Contributor Author

💃 🕺 👏 A-M-A-Z-I-N-G ! Looks great to me. 👍

Btw, this is the page I use to test various settings, for your reference: https://github.com/opendevise/asciidoc-samples/blob/master/runtime.adoc

@kivikakk
Copy link
Contributor

kivikakk commented Apr 5, 2019

Aaaah, super helpful, thank you! Glad it's looking good 👍

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.

3 participants