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

`system` method is different behavior from JRuby 1.7.x and CRuby 2.3.x #3653

Closed
hsbt opened this Issue Feb 9, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@hsbt

hsbt commented Feb 9, 2016

I'am preparing to release Rake 11. but Rake master has some blocker related JRuby 9.0.5.0.

see https://travis-ci.org/ruby/rake/jobs/107731466

I investigated 3 and 4 in above log. I found strange difference at JRuby 9.0.5.0. Please see re-produce code: https://github.com/hsbt/rake-issue-chdir

JRuby 9.0.5.0 expand ENV variables to environmental variable in system shell runner after invoking Dir.chdir.

Is this intentional behavior?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2016

Member

Ok, interesting. I'll have a look.

Member

headius commented Feb 9, 2016

Ok, interesting. I'll have a look.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2016

Member

Ok, the problem here is that the new logic for process spawning does not support chdir in the same way as MRI.

In MRI, if you specify chdir when calling spawn or system or whatever, it will actually do a native chdir before launching the child process. It does this either after forking or temporarily around the call to spawn(2) (setting it back after launching the process).

MRI also actually does a real native chdir when doing Dir.chdir.

We have never been comfortable changing the directory the JVM is running in, because we worry that will at least confuse the JVM and at worst interfere with other threads or applications running in the same process. However, we have never tested real chdir to see if anything bad happens.

A short term fix for this in JRuby would be to fall back on the old JVM process logic when we need to chdir, since that logic uses fork+exec similar to MRI. This might be ok for system, since all it does is capture all child process output. However, it would be too great a change for e.g. spawn since the JVM logic can't support the many options spawn requires.

Member

headius commented Feb 9, 2016

Ok, the problem here is that the new logic for process spawning does not support chdir in the same way as MRI.

In MRI, if you specify chdir when calling spawn or system or whatever, it will actually do a native chdir before launching the child process. It does this either after forking or temporarily around the call to spawn(2) (setting it back after launching the process).

MRI also actually does a real native chdir when doing Dir.chdir.

We have never been comfortable changing the directory the JVM is running in, because we worry that will at least confuse the JVM and at worst interfere with other threads or applications running in the same process. However, we have never tested real chdir to see if anything bad happens.

A short term fix for this in JRuby would be to fall back on the old JVM process logic when we need to chdir, since that logic uses fork+exec similar to MRI. This might be ok for system, since all it does is capture all child process output. However, it would be too great a change for e.g. spawn since the JVM logic can't support the many options spawn requires.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2016

Member

It's worth pointing out that MRI will behave the same as us even without chdir if the command given looks like a command line. For example, if I change

sh RUBY, 'check_no_expansion.rb', '$RAKE_TEST_SH', 'someval'

to

sh RUBY + ' check_no_expansion.rb $RAKE_TEST_SH someval'

MRI will also expand the env var. So it's a bit fragile to build commands that have loose env var references and expect them not to expand.

Member

headius commented Feb 9, 2016

It's worth pointing out that MRI will behave the same as us even without chdir if the command given looks like a command line. For example, if I change

sh RUBY, 'check_no_expansion.rb', '$RAKE_TEST_SH', 'someval'

to

sh RUBY + ' check_no_expansion.rb $RAKE_TEST_SH someval'

MRI will also expand the env var. So it's a bit fragile to build commands that have loose env var references and expect them not to expand.

@headius headius closed this in 1e30600 Feb 9, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Feb 9, 2016

Member

This is fixed, but it's still an issue for other spawn forms. However, I still feel like it's asking for trouble spawning subprocesses with env vars and expecting them not to expand.

Member

headius commented Feb 9, 2016

This is fixed, but it's still an issue for other spawn forms. However, I still feel like it's asking for trouble spawning subprocesses with env vars and expecting them not to expand.

@hsbt

This comment has been minimized.

Show comment
Hide comment
@hsbt

hsbt Feb 10, 2016

I still feel like it's asking for trouble spawning subprocesses with env vars and expecting them not to expand.

I agreed. Rake has expantion and no expantion tests at https://github.com/ruby/rake/blob/master/test/test_rake_file_utils.rb#L140 . I didn't know why CRuby shows this behavior.

I will discuss this to CRuby core team.

hsbt commented Feb 10, 2016

I still feel like it's asking for trouble spawning subprocesses with env vars and expecting them not to expand.

I agreed. Rake has expantion and no expantion tests at https://github.com/ruby/rake/blob/master/test/test_rake_file_utils.rb#L140 . I didn't know why CRuby shows this behavior.

I will discuss this to CRuby core team.

kares added a commit that referenced this issue Feb 15, 2016

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Mar 13, 2016

@headius @hsbt I think there's still and issue in 9.0.4.0 and Rake 11.0/11.1.

(Not sure right place to post or make issue, so thought I'd start here with a comment.)

See rails-api/active_model_serializers#1585

Excerpt for convenience:

https://travis-ci.org/rails-api/active_model_serializers/jobs/115622797

Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or do JRUBY_OPTS="-d"
/home/travis/build/rails-api/active_model_serializers/.simplecov:48: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/home/travis/.rvm/rubies/jruby-9.0.4.0/bin/jruby --verbose -w -I"lib:test" -r./test/test_helper.rb -I"/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib" "/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib/rake/rake_test_loader.rb" "test/action_controller/adapter_selector_test.rb" "test/action_controller/explicit_serializer_test.rb" "test/action_controller/json/include_test.rb" "test/action_controller/json_api/deserialization_test.rb" "test/action_controller/json_api/errors_test.rb" "test/action_controller/json_api/linked_test.rb" "test/action_controller/json_api/pagination_test.rb" "test/action_controller/serialization_scope_name_test.rb" "test/action_controller/serialization_test.rb" "test/active_model_serializers/adapter_for_test.rb" "test/active_model_serializers/json_pointer_test.rb" "test/active_model_serializers/logging_test.rb" "test/active_model_serializers/model_test.rb" "test/active_model_serializers/test/schema_test.rb" "test/active_model_serializers/test/serializer_test.rb" "test/active_record_test.rb" "test/adapter/deprecation_test.rb" "test/adapter/fragment_cache_test.rb" "test/adapter/json/belongs_to_test.rb" "test/adapter/json/collection_test.rb" "test/adapter/json/has_many_test.rb" "test/adapter/json_api/belongs_to_test.rb" "test/adapter/json_api/collection_test.rb" "test/adapter/json_api/errors_test.rb" "test/adapter/json_api/fields_test.rb" "test/adapter/json_api/has_many_embed_ids_test.rb" "test/adapter/json_api/has_many_explicit_serializer_test.rb" "test/adapter/json_api/has_many_test.rb" "test/adapter/json_api/has_one_test.rb" "test/adapter/json_api/json_api_test.rb" "test/adapter/json_api/linked_test.rb" "test/adapter/json_api/links_test.rb" "test/adapter/json_api/pagination_links_test.rb" "test/adapter/json_api/parse_test.rb" "test/adapter/json_api/relationship_test.rb" "test/adapter/json_api/relationships_test.rb" "test/adapter/json_api/resource_identifier_test.rb" "test/adapter/json_api/resource_meta_test.rb" "test/adapter/json_api/toplevel_jsonapi_test.rb" "test/adapter/json_api/type_test.rb" "test/adapter/json_test.rb" "test/adapter/null_test.rb" "test/adapter_test.rb" "test/array_serializer_test.rb" "test/collection_serializer_test.rb" "test/generators/scaffold_controller_generator_test.rb" "test/generators/serializer_generator_test.rb" "test/grape_test.rb" "test/include_tree/from_include_args_test.rb" "test/include_tree/from_string_test.rb" "test/include_tree/include_args_to_hash_test.rb" "test/lint_test.rb" "test/logger_test.rb" "test/poro_test.rb" "test/serializable_resource_test.rb" "test/serializers/association_macros_test.rb" "test/serializers/associations_test.rb" "test/serializers/attribute_test.rb" "test/serializers/attributes_test.rb" "test/serializers/cache_test.rb" "test/serializers/cached_serializer_test.rb" "test/serializers/configuration_test.rb" "test/serializers/fieldset_test.rb" "test/serializers/meta_test.rb" "test/serializers/options_test.rb" "test/serializers/root_test.rb" "test/serializers/serializer_for_test.rb" 
jruby: unknown option --verbose
rake aborted!
Command failed with status (1): [ruby --verbose -w -I"lib:test" -r./test/test_helper.rb -I"/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib" "/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib/rake/rake_test_loader.rb" "test/action_controller/adapter_selector_test.rb" 

bf4 commented Mar 13, 2016

@headius @hsbt I think there's still and issue in 9.0.4.0 and Rake 11.0/11.1.

(Not sure right place to post or make issue, so thought I'd start here with a comment.)

See rails-api/active_model_serializers#1585

Excerpt for convenience:

https://travis-ci.org/rails-api/active_model_serializers/jobs/115622797

Coverage may be inaccurate; set "cli.debug=true" ("-Xcli.debug=true") in your .jrubyrc or do JRUBY_OPTS="-d"
/home/travis/build/rails-api/active_model_serializers/.simplecov:48: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/home/travis/.rvm/rubies/jruby-9.0.4.0/bin/jruby --verbose -w -I"lib:test" -r./test/test_helper.rb -I"/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib" "/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib/rake/rake_test_loader.rb" "test/action_controller/adapter_selector_test.rb" "test/action_controller/explicit_serializer_test.rb" "test/action_controller/json/include_test.rb" "test/action_controller/json_api/deserialization_test.rb" "test/action_controller/json_api/errors_test.rb" "test/action_controller/json_api/linked_test.rb" "test/action_controller/json_api/pagination_test.rb" "test/action_controller/serialization_scope_name_test.rb" "test/action_controller/serialization_test.rb" "test/active_model_serializers/adapter_for_test.rb" "test/active_model_serializers/json_pointer_test.rb" "test/active_model_serializers/logging_test.rb" "test/active_model_serializers/model_test.rb" "test/active_model_serializers/test/schema_test.rb" "test/active_model_serializers/test/serializer_test.rb" "test/active_record_test.rb" "test/adapter/deprecation_test.rb" "test/adapter/fragment_cache_test.rb" "test/adapter/json/belongs_to_test.rb" "test/adapter/json/collection_test.rb" "test/adapter/json/has_many_test.rb" "test/adapter/json_api/belongs_to_test.rb" "test/adapter/json_api/collection_test.rb" "test/adapter/json_api/errors_test.rb" "test/adapter/json_api/fields_test.rb" "test/adapter/json_api/has_many_embed_ids_test.rb" "test/adapter/json_api/has_many_explicit_serializer_test.rb" "test/adapter/json_api/has_many_test.rb" "test/adapter/json_api/has_one_test.rb" "test/adapter/json_api/json_api_test.rb" "test/adapter/json_api/linked_test.rb" "test/adapter/json_api/links_test.rb" "test/adapter/json_api/pagination_links_test.rb" "test/adapter/json_api/parse_test.rb" "test/adapter/json_api/relationship_test.rb" "test/adapter/json_api/relationships_test.rb" "test/adapter/json_api/resource_identifier_test.rb" "test/adapter/json_api/resource_meta_test.rb" "test/adapter/json_api/toplevel_jsonapi_test.rb" "test/adapter/json_api/type_test.rb" "test/adapter/json_test.rb" "test/adapter/null_test.rb" "test/adapter_test.rb" "test/array_serializer_test.rb" "test/collection_serializer_test.rb" "test/generators/scaffold_controller_generator_test.rb" "test/generators/serializer_generator_test.rb" "test/grape_test.rb" "test/include_tree/from_include_args_test.rb" "test/include_tree/from_string_test.rb" "test/include_tree/include_args_to_hash_test.rb" "test/lint_test.rb" "test/logger_test.rb" "test/poro_test.rb" "test/serializable_resource_test.rb" "test/serializers/association_macros_test.rb" "test/serializers/associations_test.rb" "test/serializers/attribute_test.rb" "test/serializers/attributes_test.rb" "test/serializers/cache_test.rb" "test/serializers/cached_serializer_test.rb" "test/serializers/configuration_test.rb" "test/serializers/fieldset_test.rb" "test/serializers/meta_test.rb" "test/serializers/options_test.rb" "test/serializers/root_test.rb" "test/serializers/serializer_for_test.rb" 
jruby: unknown option --verbose
rake aborted!
Command failed with status (1): [ruby --verbose -w -I"lib:test" -r./test/test_helper.rb -I"/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib" "/home/travis/build/rails-api/active_model_serializers/vendor/bundle/jruby/2.2.0/gems/rake-11.1.0/lib/rake/rake_test_loader.rb" "test/action_controller/adapter_selector_test.rb" 
@hsbt

This comment has been minimized.

Show comment
Hide comment
@hsbt

hsbt Mar 13, 2016

@bf4 It's not related this issue. It's regression of rake behavior. Can you submit this issue to ruby/rake ?

I will investigate next week.

hsbt commented Mar 13, 2016

@bf4 It's not related this issue. It's regression of rake behavior. Can you submit this issue to ruby/rake ?

I will investigate next week.

@bf4

This comment has been minimized.

Show comment
Hide comment
@bf4

bf4 Mar 14, 2016

@hsbt Thanks. So, JRuby isn't supposed to have a --verbose flag?

bf4 commented Mar 14, 2016

@hsbt Thanks. So, JRuby isn't supposed to have a --verbose flag?

@hsbt

This comment has been minimized.

Show comment
Hide comment
@hsbt

hsbt Mar 14, 2016

Yes. I will fix your issue at ruby/rake#120 and release 11.1.1.

hsbt commented Mar 14, 2016

Yes. I will fix your issue at ruby/rake#120 and release 11.1.1.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 14, 2016

Member

@bf4 It appears MRI does have a --verbose flag and we don't. So I'll fix that here.

Member

headius commented Mar 14, 2016

@bf4 It appears MRI does have a --verbose flag and we don't. So I'll fix that here.

headius added a commit that referenced this issue Mar 14, 2016

headius added a commit that referenced this issue Mar 14, 2016

bf4 added a commit to bf4/rails that referenced this issue Mar 30, 2016

Run latest precompiled JRuby on CI only against ActionPack
Uses latest precompiled JRuby so that
we don't spend time downloading versions Travis has
not already compiled. http://rubies.travis-ci.org/

Uses latest jdk: oraclejdk8
per
https://docs.travis-ci.com/user/build-environment-updates/2015-02-03/#Ruby-VM
and
https://docs.travis-ci.com/user/languages/ruby/#Supported-Ruby-Versions-and-RVM

Follows on work in rails#23927 which was reverted
rails@26fe5fa

JRUBY_OPTS minimize GC, disable JIT, for max test speed
  - rails#16613
  - rails#17088

Have Rails use JRuby-compatible Rake 11.1
  - The Rake task was passing --verbose, an invalid option, to contemporary JRuby
  - ruby/rake#120
  - rails-api/active_model_serializers#1585
  - jruby/jruby#3653 (comment)

No advantage to directly mounting JRuby over installing from cache; both on S3
  - rails@b2d5b33
  - rails@f4fad04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment