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

Add GitHub Actions script #1496

Merged
merged 1 commit into from Jan 28, 2020
Merged

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented Jan 20, 2020

See workflow runs here: https://github.com/slonopotamus/asciidoctor-pdf/actions

This PR cannot be taken as-is because it exposes several problems:

  1. jaro_winkler (rubocop dependency) crashes compiler on Ruby 2.3 + Windows
  2. Several tests fail to pass on Windows. Mostly because they're trying to execute files that are not executable from Windows point of view, but there are some failures that need additional investigation.
    WRT running non-executable things, I suggest using bundle exec <gem_binary> instead of just <gem_binary>, this is more cross-platform.

@slonopotamus slonopotamus changed the title Add GitHub actions script Add GitHub Actions script Jan 20, 2020
@mojavelinux
Copy link
Member

mojavelinux commented Jan 20, 2020

Mostly because they're trying to execute files that are not executable from Windows point of view, but there are some failures that need additional investigation.

I wonder why these pass on AppVeyor.

Currently, AppVeyor only runs 2.7. At the very least, I should add 2.3 until we make the switch over (to GitHub Actions)

@mojavelinux
Copy link
Member

It turns out, the AppVeyor builds aren't even running. ooops!

@mojavelinux
Copy link
Member

WRT running non-executable things, I suggest using bundle exec <gem_binary> instead of just <gem_binary>, this is more cross-platform.

Yes and no. The problem with running the CLI tests that way is that it's not a pure test. It can produce false positives as the result of being tainted by the Bundler environment. This situation is already handled in the test suite. Some tests require the Bundler environment, others don't. By default, the Bundler environment is inherited (via environment variables) when executing a command. But the run_command helper has a way to disable this to run certain tests outside of Bundler.

@mojavelinux
Copy link
Member

I'm not ready to trust these results on Windows. Those errors, esp the CLI ones, seem odd to me. I'll need to investigate.

@mojavelinux
Copy link
Member

Hmm, looks like now that AppVeyor is running, we have similar issues.

@slonopotamus
Copy link
Contributor Author

My point is that when you call Gem.bin_path, it will give you a path to ruby file with a shebang. You cannot simply spawn a process out of that because on Windows, there is no machinery to handle shebangs.

When you're installing via bundler/gem, it creates batch wrappers, that's why you can run asciidoctor from command line:

image

For the reference, contents of these wrappers (they are all identical):

@ECHO OFF
@"%~dp0ruby.exe" "%~dpn0" %*

This "cheat" only works because of the way how cmd.exe decides what to run. It actually runs asciidoctor.bat when you type asciidoctor.

But when you're spawning processes programmatically, there's nobody in-between you and OS to use the wrapper.

@mojavelinux
Copy link
Member

My point is that when you call Gem.bin_path , it will give you a path to ruby file with a shebang. You cannot simply spawn a process out of that because on Windows, there is no machinery to handle shebangs.

Yep, I gotcha. But I had already solved this in the Asciidoctor core test suite. I just forgot about it. You need to prepend ruby (the full path) to the command.

@mojavelinux
Copy link
Member

But when you're spawning processes programmatically, there's nobody in-between you and OS to use the wrapper.

I had forgotten about that. Fortunately, my past self had already figured out a solution ;)

@slonopotamus slonopotamus force-pushed the gh-actions branch 4 times, most recently from b688680 to 222143f Compare January 21, 2020 08:17
@slonopotamus slonopotamus marked this pull request as ready for review January 21, 2020 08:27
@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 21, 2020

Okay, now it passes.

Notes:

  1. Unable to test 2.3 on Windows. Compiler crashes when compiling jaro_winkler (rubocop dependency) o_O
  2. Didn't add asciidoctor-1.5.3 to the matrix.

@mojavelinux
Copy link
Member

Unable to test 2.3 on Windows. Compiler crashes when compiling jaro_winkler (rubocop dependency)

Bizarre that this doesn't fail on AppVeyor. Then again, there are different ways to install Ruby on Windows, so perhaps the installations are just different. Though the one on AppVeyor is the 32-bit version. Maybe that matters.

.gitattributes Outdated Show resolved Hide resolved
@slonopotamus slonopotamus force-pushed the gh-actions branch 3 times, most recently from 9520477 to 1d430ba Compare January 21, 2020 10:50
@mojavelinux
Copy link
Member

Now that I got AppVeyor up and running, we can compare runtimes w/ GitHub Actions. Given that we are so resource constrained on AppVeyor, having GitHub Actions handle Windows builds seems to be an easy choice. Travis is really slow at running the Linux builds, though it does allows us to install the packages we need to test the gs integration.

I'm not convinced the macOS builds are necessary. There is really 0 difference in how a gem runs on macOS vs Linux. That just seems like overhead. I could maybe see running one Ruby just to touch it, but it's not going to add much value otherwise.

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 21, 2020

Dropped MacOS from matrix: https://github.com/slonopotamus/asciidoctor-pdf/commit/ee3d16faa6ddff4edecd334b2c4e7a4fd333870c/checks

I'm not going to persuade you into using GitHub Actions. Even if the only effect of this PR is that you've fixed AppVeyor builds, this is good enough for me.

It is unfortunate that Ruby 2.3+Windows fails, this needs to be reported to jaro_winkler project.

@mojavelinux
Copy link
Member

I'm not going to persuade you into using GitHub Actions.

Oh, I think we should use it. I'm just trying to determine how much of it and when. I'm really not worried about the Windows Ruby 2.3 issue since 2.3 on Windows is really bad anyway. We can just test 2.4.

@slonopotamus slonopotamus force-pushed the gh-actions branch 3 times, most recently from eb58b84 to 7dd65e6 Compare January 25, 2020 12:36
@mojavelinux
Copy link
Member

Aha, I see the problem:

bundle install --jobs 4 --retry 3

That needs to be:

bundle install --path=.bundle/gems --jobs 4 --retry 3

The tests currently assume that the gems are installed inside the project.

@mojavelinux
Copy link
Member

No, that's not it. The issue is that GitHub Actions doesn't use RVM. And there are subtle differences about where RVM installs things.

@mojavelinux
Copy link
Member

mojavelinux commented Jan 26, 2020

I revved up a Docker image and duplicated what GitHub Actions does. It's doing something non-standard with Bundler (or it's the behavior of Bundler that changes depending on how you invoke it). The bin script for the current project is not getting installed in the bin directory, which is exactly what the test is trying to validate.

This is solved by adding --path .bundle/gems when invoking bundler. That puts it into the right mode.

@mojavelinux
Copy link
Member

The jaro_winkler dependency is a misery. We could move rubocop to a group in the gemfile that can be excluded. It only needs to run on one build anyway.

.gitattributes Outdated Show resolved Hide resolved
@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 26, 2020

Current state: everything passes except 4 tests on JRuby+Windows.

I think I'll split this PR since test fixes are not GH-Actions specific and can also be reproduced outside of it.

@slonopotamus
Copy link
Contributor Author

This PR depends on #1523.

@slonopotamus
Copy link
Contributor Author

@slonopotamus
Copy link
Contributor Author

Latest run: fully passes

@slonopotamus slonopotamus force-pushed the gh-actions branch 2 times, most recently from 363bb7c to 4709b39 Compare January 27, 2020 20:33
@mojavelinux
Copy link
Member

I think all the prerequisites are now covered.

For now, I'd like to only use GitHub Actions for what Travis doesn't already cover (basically to replace AppVeyor). So that would be Windows only.

Another possibility is to run 6 combinations.

  • JRuby on Windows
  • JRuby on Linux
  • 2.4 on Windows
  • 2.4 on Linux
  • 2.7 on Windows
  • 2.7 on Linux

That way we are touching all the boundaries. The more fine-grained testing of Ruby versions happens in Travis.

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 27, 2020

Matrix can be adjusted as you want it to be. Changed to Win-only.

MRI jobs completed in 3m20s, JRuby in 4m15s. Ubuntu is obviously faster, around 1m45s.

@slonopotamus
Copy link
Contributor Author

No, wait. GH-Actions runs 922 tests, while AppVeyor runs 934. Some are disabled on Ruby-2.7?

@mojavelinux
Copy link
Member

You need to enable some environment variables:

https://github.com/asciidoctor/asciidoctor-pdf/blob/master/.travis.yml#L11-L12

Note that rghost won't likely work on Windows.

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Jan 28, 2020

Note that rghost won't likely work on Windows.

You can install Ghostscript, but rghost needs some setup in order to find it. Possibly, by adding a env var that would set RGhost::Config::GS[:path].

      # Ghostscript will be available at C:\Program Files\gs\gs9.50\bin\gswin64.exe
      - name: Set up Ghostscript
        uses: crazy-max/ghaction-chocolatey@v1.0.2
        with:
          args: install --no-progress ghostscript

Unfortunately, Ghostscript binaries dir is not added to PATH. This is reported as chocolatey-community/chocolatey-packages#1330.

@mojavelinux
Copy link
Member

I'm ready to move forward with this. I'll probably continue to tweak it. Thanks for getting it started!

@mojavelinux mojavelinux merged commit c62197c into asciidoctor:master Jan 28, 2020
@slonopotamus slonopotamus deleted the gh-actions branch January 28, 2020 11:16
@slonopotamus
Copy link
Contributor Author

Nooooooo! :D You also hit asciidoctor/asciidoctor-epub3#253 I didn't notice that last run that triggered after I enabled pygments.rb didn't actually finish but is still running.

@slonopotamus
Copy link
Contributor Author

Created #1525 to disable pygments.rb on JRuby+Windows.

@mojavelinux
Copy link
Member

No problem. I'll just fix in master.

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

2 participants