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 relativizing directory on remotes that do not represent a path #205

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

aymeric-ledorze
Copy link
Contributor

Hi.

While using the latest version of appraisal in one of my projects running in a docker container, I noticed that I could not run the application because the lock file was corrupt. Indeed, instead of remote: https://github.com/thoughtbot/appraisal, the lock file contained remote: https://github.com/thoughtbot..raisal.

I traced the error back to the relativize method of Appraisal::Appraisal:

def relativize
current_directory = Pathname.new(Dir.pwd)
relative_path = current_directory.relative_path_from(gemfile_root).cleanpath
lockfile_content = File.read(lockfile_path)
File.open(lockfile_path, 'w') do |file|
file.write lockfile_content.gsub(/#{current_directory}/, relative_path.to_s)
end
end

It replaces an absolute path by a relative one. In my case, my app was running in a /app folder, and therefore the "/app" part of "thoughtbot/appraisal" is replaced by "..".

This PR solves this issue by replacing only the absolute paths that are preceded by a space. However, this may introduce a regression since I could not make any test of the current test suite fail when redefining the relativize method to just copy the content of the lock file without changing it. Therefore, I wonder if relativize tries to solve an issue that cannot be reproduced anymore.

Appraisal was creating invalid lock files, because paths weren't being
relatived correctly. e.g.:

    remote: https://github.com/thoughtbot..raisal

Instead of:

    remote: https://github.com/thoughtbot/appraisal

This PR solves this issue by replacing only the absolute paths that are
preceded by a space.
@nickcharlton
Copy link
Member

Wonderful, thank you for noticing this and providing a fix.

@nickcharlton nickcharlton merged commit ac72bac into thoughtbot:main Feb 6, 2023
@mogman1
Copy link

mogman1 commented Mar 10, 2023

@nickcharlton Sorry to be a bother, but I just ran into this as well with docker. I pulled the master branch and confirmed this change fixes things for me as well. Any idea when this might be formally released?

@nickcharlton
Copy link
Member

No worries — we're stuck behind some bundler compatibility issues which are showing up in setting up CI again. We're tracking that here: #204.

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.

3 participants