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

Fix error when trying to find bundler with a deleted "working directo… #3090

Merged
4 commits merged into from
Feb 13, 2020

Conversation

bronzdoc
Copy link
Member

closes #3087

Description:

Fix and error in the Bundler version finder when run in a deleted current working directory


I will abide by the code of conduct.

…ry" - `pwd': No such file or directory - getcwd (Errno::ENOENT)
Copy link

@asf-stripe asf-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting in a super quick fix to this! I left some comments from that I gathered as I investigated this issue - please use or discard them as you see fit (:

Dir.chdir(dir)
end

assert_nil bvf.bundler_version_with_reason

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test probably should chdir back to its old dir, otherwise the other tests are going to have a hard time / fail in an order-dependent way.

Copy link
Member

@duckinator duckinator Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bronzdoc can you elaborate on why you marked this as resolved? I have the same concern as @asf-stripe, but I may be missing some context. (E.g., does the setup process run before each test call Dir.chdir, or something like that?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both of you! I fixed it. It will restore to the original PWD, somehow I thought I was passing that block to Dir.chdir...

lib/rubygems/bundler_version_finder.rb Outdated Show resolved Hide resolved
@bronzdoc bronzdoc force-pushed the fix/bundler-version-finder-pwd branch from 4187449 to e4df2de Compare January 25, 2020 01:54
@bronzdoc
Copy link
Member Author

For some reason the CI Windows environment can't delete the tmpdir of the test I added for this change - Errno::EACCES: Permission denied @ dir_s_rmdir - C:/Users/RUNNER~1/AppData/Local/Temp/d20200125-5128-70ojio

@MSP-Greg Have this happened to you in the past? maybe you can point me to the right track 🙇

@MSP-Greg
Copy link
Contributor

@bronzdoc

Have this happened to you in the past?

Similar. I've debated whether to track down the issue or not.

Windows uses TEMP and TMP for ENV settings to get the temp folder. The path is C:/Users/runneradmin/AppData/Local/Temp, but since runneradmin is a 'long path name', it's 'short name' of RUNNER~1 is used, probably for compatibility with legacy apps.

What I've done when it's an issue is set TMPDIR to a folder. TMPDIR is the first ENV value checked by Ruby. From years ago, I've also set up my local systems that way...

Maybe something like:

      - name: Run Test
        run: |
          mkdir temp
          $env:TMPDIR = "$pwd/temp"
          ridk enable
          rake test

@MSP-Greg
Copy link
Contributor

@bronzdoc

I just looked at the test that's causing the issue.

On Windows, one can't delete the dir that is $pwd / Dir.pwd. Nothing to do with temp files or directories...

@bronzdoc
Copy link
Member Author

Thanks so much for looking into this @MSP-Greg!
I guess I would have to explicitly create and remove a dir to test this then. 🤷‍♂

@MSP-Greg
Copy link
Contributor

@bronzdoc

I'm probably missing something, but I think the test won't work on Windows and, hence, needs to be skipped.

As one can't delete $pwd, one also can't Dir.chdir into a non-existent directory...

…pwd/Dir.pwd. Nothing to do with temp files or directories
Copy link
Member

@duckinator duckinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, aside from the same issue that was mentioned by @asf-stripe — I'm wondering if you may need to Dir.chdir back to where it was prior to the test running.

Dir.chdir(dir)
end

assert_nil bvf.bundler_version_with_reason
Copy link
Member

@duckinator duckinator Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bronzdoc can you elaborate on why you marked this as resolved? I have the same concern as @asf-stripe, but I may be missing some context. (E.g., does the setup process run before each test call Dir.chdir, or something like that?)

@bronzdoc
Copy link
Member Author

@bundlerbot r+

ghost pushed a commit that referenced this pull request Feb 13, 2020
3090: Fix error when trying to find bundler with a deleted "working directo… r=bronzdoc a=bronzdoc

closes #3087

# Description:
Fix and error in the Bundler version finder when run in a deleted `current working directory` 
______________

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


3127: Refactor ruby path finding r=bronzdoc a=deivid-rodriguez

# Description:

We have some duplicated logic to find out the fullpath to the ruby binary. I think `RbConfig.ruby` can be fully trusted as the source of truth for this information.

# Tasks:

- [x] Describe the problem / feature
- [ ] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).


Co-authored-by: bronzdoc <lsagastume1990@gmail.com>
Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link

ghost commented Feb 13, 2020

Build succeeded

  • install (2.3.8)
  • install (2.4.9)
  • install (2.5.7)
  • install (2.6.5)
  • install (jruby-9.2.9.0)
  • macos (2.4.x)
  • macos (2.5.x)
  • macos (2.6.x)
  • ruby_master
  • ubuntu (2.4.x, bundler)
  • ubuntu (2.4.x, rubygems)
  • ubuntu (2.5.x, bundler)
  • ubuntu (2.5.x, rubygems)
  • ubuntu (2.6.x, bundler)
  • ubuntu (2.6.x, rubygems)
  • ubuntu_bundler_master (2.6.x)
  • ubuntu_lint
  • ubuntu_rvm (2.3.8)
  • ubuntu_rvm (jruby-9.2.9.0)
  • ubuntu_rvm (ruby-head)
  • windows (2.4.x)
  • windows (2.5.x)
  • windows (2.6.x)

@ghost ghost merged commit ad5bff2 into master Feb 13, 2020
@ghost ghost deleted the fix/bundler-version-finder-pwd branch February 13, 2020 15:37
This pull request was closed.
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.

Bundler version finder breaks when run in deleted current working dir
4 participants