Enable Windows support #319

jpogran · 2020-09-05T03:24:38Z

This PR enables Windows platform support by adding Windows platforms to the supported? methods in compile_cache and in load_path_cache and adds Github Action yaml files to test these changes on windows, linux, and macos.

It also changes the Rakefile to use the minitest rakefile methods to run all of the tests. This is done because the existing bin/testunit calls directly to sh which is not present on Windows platforms. By making this a ruby/rake call it makes the test suite platform independent. The existing files in bin are left in case there is other existing infrastructure that uses them.

Lastly it also makes several changes to the existing test files to support the Windows platform. These changes mainly comprise using Windows file paths or file access methods. There are two tests called out that do not work on Windows at all, and are hardcoded to pass. I would have used 'pending', but I could not find out how to do that in minitest.

Many thanks to @daniel-rikowski who originally got the tests to pass in the first place, and I would not have got anywhere without.

Fixes #316

jpogran · 2020-09-05T03:26:43Z

test/compile_cache_key_format_test.rb

+      # Always pass this test on Windows, because I can't find an equivalent for
+      # /dev/null for this test
+      # I thought this would work but it fails to find it
+      # if RbConfig::CONFIG['host_os'] =~ /mswin|mingw|cygwin/
+      #   target = "NUL:"
+      # else
+      #   target = '/dev/null'
+      # end
+      pass


I tried using NUL: but it doesn't recognize it as a valid path. I'll ask around if there is another approach I can use.

Have you tried NUL, i.e. without the colon? NUL, CON, etc. are more like files_, not paths or drives. But still, most of the regular Win32 operations will most likely fail...

So, this turned out to partially be a red herring. /dev/null wasn't a path to write to, it was example text to write to the file and then test it was read correctly, so it doesn't actually matter what the content is.

The actual problem was that "#{@tmp_dir}/8c/d2d180bbd995df" isn't actually created on windows. It took some experimentation and iterating through all files in the entire cache dir and reading each one to find "#{@tmp_dir}/36/9eba19c29ffe00" was the file created by this method. I don't know if this is just an artifact of the implementation on Windows (different paths) or that this is a bug in general, but now the test works correctly and passes.

jpogran · 2020-09-05T03:27:10Z

test/compile_cache_test.rb

+    if RbConfig::CONFIG['host_os'] =~ /mswin|mingw|cygwin/
+      # Always pass this test on Windows, because it is not easily possible to make a directory read-only,
+      # i.e. FileUtils.chmod(0400, ...) has not the desired effect, neither does a call to system('attrib +R ...').
+      pass
+    else


I'll ask around to see if there is an in-code way we can do this on Windows

Well, there is the ACL api: You could remove the current user's access rights, e.g by using SetSecurityInfo But IMHO this is such a weird scenario on a Windows platform, it doesn't warrant that much effort. Still, if you find a good way, it would be very interesting for joeyates/imap-backup#78 😄

After taking the time to read @daniel-rikowski's post and walking through the tests in this class, I think I understand the purpose for this test better, and this it doesn't apply on Windows. I've set the test to pass on Windows, as directories aren't 'read' like they are on Linux. You can restrict the ability to list files in a directory or restrict ACLs on the directory to prevent listing. Since the purpose of this is more to test we can still read files, I feel confident that skipping this test doesn't compromise testing reading files on Windows.

jpogran · 2020-09-05T03:38:49Z

You can't see the github action working here because it's not committed yet, but I had it running on my fork: https://github.com/jpogran/bootsnap/actions/runs/240329764.

jpogran · 2020-09-08T14:49:16Z

@burke If you would like me to split this PR into two, one for enabling github actions and another for enabling windows platform support, LMK. This way you can see the github actions working on the existing suite for linux, so my modifications can be compared.

This commit enables Windows platform support by adding Windows platforms to the `supported?` methods in `compile_cache` and in `load_path_cache`.

jpogran · 2020-09-30T21:16:04Z

Hi @burke , I've updated the PR and all tests pass, if you could review this I would appreciate it.

If you want to ensure the tests pass on all platforms before merging my changes, I can split this PR into two and you can merge the Github Actions first, which will then test my changes in a second PR. Otherwise, the travis check shows it still works on linux and I pasted output from my Windows machine below:

❯ be rake
install -c tmp/x64-mingw32/bootsnap/2.5.8/bootsnap.so lib/bootsnap/bootsnap.so
cp tmp/x64-mingw32/bootsnap/2.5.8/bootsnap.so tmp/x64-mingw32/stage/lib/bootsnap/bootsnap.so
Run options: --seed 1490

# Running:

.........................................................................................................................................

Finished in 1.672203s, 81.9279 runs/s, 160.2677 assertions/s.

137 runs, 268 assertions, 0 failures, 0 errors, 0 skips

burke

Sorry for the delay. I was off for a while. A couple of trivial changes but I'm happy to merge this otherwise. Thanks for putting in the work on this!

burke · 2020-10-05T14:08:04Z

Rakefile

@@ -10,8 +10,17 @@ Rake::ExtensionTask.new do |ext|
  ext.gem_spec = gemspec
 end

-task :test do
-  sh 'bin/testunit'
+# task :test do


remove commented code

burke · 2020-10-05T14:08:14Z

test/compile_cache_key_format_test.rb

+
+
+
+# Dir.glob("#{d}/**/*") do |file|


remove commented code

d'oh. Leftover when I was trying to figure out which file for the cache test. Removed!

This commit changes the Rakefile to use the minitest rakefile methods to run all of the tests. This is done because the existing `bin/testunit` calls directly to `sh` which is not present on Windows platforms. By making this a ruby/rake call it makes the test suite platform independent. The existing files in `bin` are left in case there is other existing infrastructure that uses them. This commit also adds a Github Action yaml file that tests this code against the latest versions of windows, ubuntu, and macos. This validates that the test runner change works on all of the existing supported platforms as well as Windows.

This commit makes several changes to the existing test files to support the Windows platform. These changes mainly comprise using Windows file paths or file access methods. There are two tests called out that do not work on Windows at all, and are hardcoded to pass. I would have used 'pending', but I could not find out how to do that in minitest. Co-authored-by: Daniel Rikowski <[email protected]>

Add documentation on how to run the tests on Windows platforms.

ghost added the cla-needed label Sep 5, 2020

jpogran commented Sep 5, 2020

View reviewed changes

jpogran force-pushed the windows-support branch 2 times, most recently from c9b83fa to 375929f Compare September 5, 2020 03:37

jpogran force-pushed the windows-support branch from 375929f to 892f513 Compare September 8, 2020 14:47

Enable Windows support

fae33c3

This commit enables Windows platform support by adding Windows platforms to the `supported?` methods in `compile_cache` and in `load_path_cache`.

jpogran force-pushed the windows-support branch from 808b645 to 04ccfbf Compare September 15, 2020 01:01

ghost removed the cla-needed label Sep 15, 2020

jpogran force-pushed the windows-support branch from 04ccfbf to 8defd1f Compare September 30, 2020 19:58

jpogran marked this pull request as ready for review September 30, 2020 20:00

jpogran requested a review from burke as a code owner September 30, 2020 20:00

jpogran changed the title ~~(WIP) Enable Windows support~~ Enable Windows support Sep 30, 2020

burke approved these changes Oct 5, 2020

View reviewed changes

jpogran and others added 3 commits October 5, 2020 12:03

(maint) Add Windows setup steps to CONTRIBUTING

32394ff

Add documentation on how to run the tests on Windows platforms.

jpogran force-pushed the windows-support branch from 63c2610 to 32394ff Compare October 5, 2020 16:03

burke merged commit 057c994 into Shopify:master Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Windows support #319

Enable Windows support #319

jpogran commented Sep 5, 2020 •

edited

Loading

jpogran Sep 5, 2020

daniel-rikowski Sep 5, 2020 •

edited

Loading

jpogran Sep 30, 2020

jpogran Sep 5, 2020

daniel-rikowski Sep 5, 2020

jpogran Sep 30, 2020

jpogran commented Sep 5, 2020

jpogran commented Sep 8, 2020

jpogran commented Sep 30, 2020

burke left a comment

burke Oct 5, 2020

burke Oct 5, 2020

jpogran Oct 5, 2020

Enable Windows support #319

Enable Windows support #319

Conversation

jpogran commented Sep 5, 2020 • edited Loading

Choose a reason for hiding this comment

daniel-rikowski Sep 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpogran commented Sep 5, 2020

jpogran commented Sep 8, 2020

jpogran commented Sep 30, 2020

burke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpogran commented Sep 5, 2020 •

edited

Loading

daniel-rikowski Sep 5, 2020 •

edited

Loading