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 symlink regression #541

Merged
merged 5 commits into from
Sep 10, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions lib/react_on_rails/assets_precompile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ def symlink_file(target, symlink)
target_exists = File.exist?(target_path)
raise SymlinkTargetDoesNotExistException, "Target Path was: #{target_path}" unless target_exists

# File.exist?(symlink_path) will check the file the sym is pointing to is existing
# File.lstat(symlink_path).symlink? confirms that this is a symlink
valid_symlink_already_exists = File.exist?(symlink_path) && File.lstat(symlink_path).symlink?

if valid_symlink_already_exists
if symlink_and_points_to_existing_file?(symlink_path)
puts "React On Rails: Digested version of #{symlink} already exists indicating #{target} did not change."
return
end
Expand Down Expand Up @@ -120,7 +116,15 @@ def clobber

private

def symlink_and_points_to_existing_file?(symlink_path)
# File.exist?(symlink_path) will check the file the sym is pointing to is existing
# File.lstat(symlink_path).symlink? confirms that this is a symlink
File.exist?(symlink_path) && File.lstat(symlink_path).symlink?
end

def file_or_symlink_exists_at_path?(path)
# We use lstat and not stat, we we don't want to visit the file that the symlink maybe
# pointing to. We can't use File.exist?, as that would check the file pointed at by the symlink.
File.lstat(path)
true
rescue
Expand Down
42 changes: 42 additions & 0 deletions spec/react_on_rails/assets_precompile_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,48 @@ module ReactOnRails
end.to raise_exception(AssetsPrecompile::SymlinkTargetDoesNotExistException)
end
end

it "creates a proper symlink when a file exists at destination" do
Copy link
Contributor Author

@robwise robwise Sep 8, 2016

Choose a reason for hiding this comment

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

you should specify which destination you are talking about

filename = File.basename(Tempfile.new("tempfile", assets_path))
existing_filename = File.basename(Tempfile.new("tempfile", assets_path))
digest_filename = existing_filename

AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

usually we have a line between the action taken and the expectation

expect(assets_path.join(digest_filename).lstat.symlink?).to be true
expect(File.identical?(assets_path.join(filename),
assets_path.join(digest_filename))).to be true
end

it "creates a proper symlink when a symlink file exists at destination" do
Copy link
Contributor Author

@robwise robwise Sep 8, 2016

Choose a reason for hiding this comment

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

is it a valid symlink or invalid symlink? It doesn't do anything if it's valid, just outputs a message

context "when there is already a valid symlink in place" do
   it "outputs a message saying that it need not perform any action" do

filename = File.basename(Tempfile.new("tempfile", assets_path))
existing_filename = File.basename(Tempfile.new("tempfile", assets_path))
digest_file = Tempfile.new("tempfile", assets_path)
digest_filename = File.basename(digest_file)
File.delete(digest_file)
File.symlink(existing_filename, digest_filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get what's going on here compared to your test description


AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename)
expect(assets_path.join(digest_filename).lstat.symlink?).to be true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

usually we have a line between the action taken and the expectation

expect(File.identical?(assets_path.join(filename),
assets_path.join(digest_filename))).to be true
File.delete(digest_filename)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary, this is a TempFile

end

it "creates a proper symlink when an invalid symlink exists at destination" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

context "when there is an existing invalid symlink" do
  it "replaces it with a valid one" do

filename = File.basename(Tempfile.new("tempfile", assets_path))
existing_file = Tempfile.new("tempfile", assets_path)
existing_filename = File.basename(existing_file)
digest_file = Tempfile.new("tempfile", assets_path)
digest_filename = File.basename(digest_file)
File.symlink(existing_filename, digest_filename)
File.delete(existing_file) # now digest_filename is an invalid link

AssetsPrecompile.new(assets_path: assets_path).symlink_file(filename, digest_filename)
expect(assets_path.join(digest_filename).lstat.symlink?).to be true
expect(File.identical?(assets_path.join(filename),
assets_path.join(digest_filename))).to be true
File.delete(digest_filename)
Copy link
Contributor Author

@robwise robwise Sep 8, 2016

Choose a reason for hiding this comment

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

why are you doing this, it's a TempFile?

end
end

describe "symlink_non_digested_assets" do
Expand Down