Conversation
ilyazub
left a comment
There was a problem hiding this comment.
Thanks for adding Windows to the CI. We can't drop Ruby 2.7 support, unfortunately.
I'm sorry, I didn't replace the mkfifo yet (ref: #38 (comment)).
|
I see, let me remove the commit dropping support for Ruby 2.7 then! |
5818840 to
df946e7
Compare
ilyazub
left a comment
There was a problem hiding this comment.
Thanks, @deivid-rodriguez!
Let's just replace FIFO in lib/turbo_tests/runner.rb with RSPEC_FORMATTER_OUTPUT_ID for all platforms. (I'm working on this now, but feel free to give it a try too. :-))
lib/turbo_tests/windows.rb
Outdated
| env["RSPEC_FORMATTER_OUTPUT_ID"] = SecureRandom.uuid | ||
| env["RUBYOPT"] = ["-I#{File.expand_path("..", __dir__)}", ENV["RUBYOPT"]].compact.join(" ") | ||
|
|
||
| command_name = Gem.win_platform? ? [Gem.ruby, "bin/rspec"] : "bin/rspec" |
There was a problem hiding this comment.
What's the best way to detect Bundler binstubs and/or if bundle exec is used?
There was a problem hiding this comment.
Good point too! Using bin/rspec directly is quite specific to our environment. I guess we could detect whether there's a bin/rspec file relative to the cwd, and otherwise use bundle exec rspec?
There was a problem hiding this comment.
I guess we could detect whether there's a
bin/rspecfile relative to the cwd, and otherwise usebundle exec rspec
Will Windows correctly handle this if we append $PWD/bin to ENV["PATH"] instead of detecting bin/rspec? (I'm new to Bundler internals. Below is a hacky pseudo-code.)
env["PATH"] << ":#{File.expand_path("bin", __dir__)}"
command_name = ENV["BUNDLE_BIN_PATH"] ? [ENV["BUNDLE_BIN_PATH"], "exec", "rspec"] : "rspec"There was a problem hiding this comment.
I think it would handle it fine, but I'm not sure it's a good idea since it would have global side effects. Users have other scripts in bin/ and maybe won't expect turbo_tests to automatically set things up so that those other scripts start getting selected when their tests run processes? Not sure.
Maybe we can start leaving things as they are regarding building the command name for now. I could configure $LOAD_PATH on our side before launching turbo_tests so that our binstub gets properly loaded.
ilyazub
left a comment
There was a problem hiding this comment.
I unified start_subprocess code for all platforms in b4ad8f4 and resolved conflicts with the master branch in 8af698e.
@deivid-rodriguez Please let me know if these changes work for you and look good to you.
|
Thanks for that! I will give this patch a try and confirm it works for us! |
|
I tried this branch through ruby/rubygems@a4e3012 and Windows CI is cool with it. I also tried it locally on Linux and found no issues either, so this seems good! Unfortunately we're having an unrelated issue, and we had to pin to 2.2.0 for now, but this seems good to me regardless! |
|
Sounds good, thank you @deivid-rodriguez. I'll merge this PR and will also investigate ruby/ruby#10496. |
|
@deivid-rodriguez @ilyazub I created #53 to resolve the OpenStruct issue (from #49) by replacing it with a Struct instead. I believe with that change, the json gem dependency could be removed since it was added in #49 along with OpenStruct. |
ilyazub
left a comment
There was a problem hiding this comment.
I merged origin/master to this branch to remove the ostruct gem dependency.
|
Thank you! Hopefully we'll be able to upgrade and remove our monkeypatches soon! |
|
Yeah, that should workaround the issue for us, thank you! In any case, I think the ruby-core issue reveals some problem in bundler or ruby-core test setup, and I believe it's better to explicitly list dependencies that these days are maintained as separate gems. This change is good for us since it unblock us from upgrading but if I manage to figure out the problem, I'll make sure to get back here and try to add back the explicit json dependency. |
|
@deivid-rodriguez What does the |
|
We try to explain it below. Basically means that the previous installation path has insecure world-writable permissions, and since Bundler needs to delete it for That all said, I don't know how or why the error is being triggered here 😮. |
We've been running turbo_tests in RubyGems with a monkeypatch to make Windows support work. A unified solution would be better but I guess getting things at least working should take priority so maybe there's interest in the raw patch.
Windows tests don't really catch any issues (they pass without this patch), although turbo_tests does not really work on Windows. In any case, as a minimal improvement over what we have, I added Windows to CI.
Finally, I dropped support for Ruby 2.7 since it reached its End of Life and also started testing with Ruby 3.3.
Happy to extract any of these changes to separate PRs!