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

Windows load path caching support #316

Closed
jpogran opened this issue Aug 11, 2020 · 8 comments · Fixed by #319
Closed

Windows load path caching support #316

jpogran opened this issue Aug 11, 2020 · 8 comments · Fixed by #319

Comments

@jpogran
Copy link
Contributor

jpogran commented Aug 11, 2020

👋 Thanks for making this! My team is investigating using this to speed up a command line Ruby app, and I noticed that the compile_cache and load_path_cache were both artificially restricted from running on Windows platforms. We are particularly interested in $LOAD_PATH caching, but the compile cache is also useful for us. Adding Windows to the supported? method results in the expected speed gains. Is there a reason that this can't be enabled on windows?

On a side note I went to run the test suite using the rakefile, but saw the explicit sh calls and therefore can't run the test suite on Windows. I would love to add tests for Windows explicitly but quickly got overwhelmed trying to figure out how to make the existing suite run on Windows.

Would you accept a PR adding Windows as supported? I realize that this could add a burden for an untested platform, but I am willing to help write tests if the harness can be modified to run in Windows environments. With Github Actions, you wouldn't have to have a Windows runner in your environment to validate the code.

➜ git diff
diff --git a/lib/bootsnap/compile_cache.rb b/lib/bootsnap/compile_cache.rb
index 266ba16..1d7c69f 100644
--- a/lib/bootsnap/compile_cache.rb
+++ b/lib/bootsnap/compile_cache.rb
@@ -34,9 +34,9 @@ module Bootsnap
     end

     def self.supported?
-      # only enable on 'ruby' (MRI), POSIX (darwin, linux, *bsd), and >= 2.3.0
+      # only enable on 'ruby' (MRI), POSIX (darwin, linux, *bsd, windows), and >= 2.3.0
       RUBY_ENGINE == 'ruby' &&
-      RUBY_PLATFORM =~ /darwin|linux|bsd/ &&
+      RUBY_PLATFORM =~ /darwin|linux|bsd|mswin|mingw/ &&
       Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("2.3.0")
     end
   end
diff --git a/lib/bootsnap/load_path_cache.rb b/lib/bootsnap/load_path_cache.rb
index c58f25d..ebcedbe 100644
--- a/lib/bootsnap/load_path_cache.rb
+++ b/lib/bootsnap/load_path_cache.rb
@@ -61,7 +61,7 @@ module Bootsnap

       def supported?
         RUBY_ENGINE == 'ruby' &&
-        RUBY_PLATFORM =~ /darwin|linux|bsd/
+        RUBY_PLATFORM =~ /darwin|linux|bsd|mswin|mingw/
       end
     end
   end
@daniel-rikowski
Copy link
Contributor

I agree, this check shouldn't be there, since Bootsnap on Windows was working well until this check was added.

Related: #60

@jpogran
Copy link
Contributor Author

jpogran commented Aug 19, 2020

Hi @burke, you're listed as maintainer in the CODEOWNERS file, are you a good person to ask about this?

@burke
Copy link
Member

burke commented Aug 26, 2020

I'm definitely happy to merge a PR that unlocks windows support as long as we also get Windows CI somehow. I have zero experience targeting Windows, but keeping CI green is pretty doable.

@daniel-rikowski
Copy link
Contributor

I'm willing to give this a try.

After I got the test suite running, I noticed that some tests implicitly assume a Linux platform. (The test suite, not Bootsnap itself, as far as I can tell) For example that /dev/null or /etc/hosts exists or that /some/path is an absolute path.

It looks like I have to sprinkle some guard clauses over the test suite...

@daniel-rikowski
Copy link
Contributor

daniel-rikowski commented Aug 27, 2020

I have modified the test suite to successfully run on Windows. Even the symlink related stuff is working (with Ruby 2.7, the older ones require admin privileges to create symlinks) As expected no other changes to the Bootsnap source were necessary.

(daniel-rikowski@7a85ed7 Ignore the changes in travis.yml, they are wrong/useless)

But I don't know how to get Windows CI...

Travis doesn't run Ruby on Windows, I get this error:

The language 'ruby' is currently unsupported on the Windows Build Environment.

Let us know if you'd like to see it: https://travis-ci.community/c/environments/windows. Thanks for understanding!

The only possible workaround is to test "bash" instead of Ruby and then manually execute the required Ruby command lines from a handcrafted bash script. But that sounds like a maintenance nightmare (and a lot of work).

On the other hand AppVeyor should work, but I never used it and I don't want to delve into that at the moment.

Is the Windows CI a hard requirement? Or would you accept a pull request even though there's no solution to Windows CI yet?

@jpogran
Copy link
Contributor Author

jpogran commented Aug 27, 2020

Thanks @daniel-rikowski! I had similar experiences you did with the test harness and with parts of the tests themselves, but had not made as much progress as you did. How did you handle executing the suite? I used minitest's example rake task to avoid the sh calls but ran into the same problems you did with each test.

As for which CI to run on, I have some experience there that might help. While Travis does not have ruby pre-installed, I do maintain a few projects where we install the ruby version we need before we run the tests, so that could work there. I also have experience with appveyor, and the same approach could work but that adds another account for the maintainers to have to sign up and maintain going forward and it would only be for windows.

I suggested in the original post to use Github Actions, as they have runners for the three platforms we would be targeting (linux, mac and windows) and they come pre-installed with alot of the software we need out of the box. If you look at windows 2019 you'll see ruby 2.5.8 is pre-installed, but it wouldn't take much to get another version installed as well. It also comes with cmake installed which could help with the compile step in the rake task, but I admit I haven't looked into how the C part of this project is built. I recently setup a test, build and release pipeline with GH Actions for a project I own, and found using GH Actions easier than travis and appveyor.

Do you have a preference @burke ?

@daniel-rikowski
Copy link
Contributor

Honestly I used the Rubymine test runner 🤣 But executing this in cmd does work, too:

ruby -I"test" -w -e "Dir.glob('./test/**/*_test.rb').each { |f| require f }"

Setting up CI most likely will reveal/create some more problems. Building the C extension requires the MSYS2/RubyInstaller2 build tools.
Also symlinks might prove difficult. They are available since Windows Vista, but until Windows 10 Creators Update they required admin privileges. And even on Windows 10 an application has to give a flag to allow this. Ruby 2.7 had it added only a year ago (https://git.ruby-lang.org/ruby.git/commit/?id=574a9edfb3) and I'm not sure if that will be backported to Ruby 2.5 and 2.6.

Funny, it's the tests which make trouble on Windows, not Bootsnap itself...

If @burke agrees, I'll create a pull request for what I have (after removing the useless travis stuff) and then Windows CI can be added in a future step.

@jpogran
Copy link
Contributor Author

jpogran commented Sep 5, 2020

I was able to take most of @daniel-rikowski's test changes and add a Github action that tests on all platforms. The GH Action uses an action that installs and sets up Ruby with the Devkit on windows, so I was able to compile the native gem and run the tests. There are still two tests to address so I left the PR in draft.

@burke burke closed this as completed in #319 Oct 6, 2020
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 a pull request may close this issue.

3 participants