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.

Sign up for GitHub

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

Jump to bottom

Copy dir if symlink fails #7447

Merged
merged 6 commits into from
Aug 9, 2023
Merged

Copy dir if symlink fails #7447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28875a6
Copy dir if symlink fails
anjutiwari Apr 24, 2023
ffa6d01
Copy dir if symlink fails
anjutiwari Apr 24, 2023
56087c4
Merge branch 'main' into CT-2457/dbt-deps
anjutiwari Jun 30, 2023
a4d28cf
Update .changes/unreleased/Fixes-20230424-210734.yaml
gshank Aug 8, 2023
1bbd247
Merge branch 'CT-2457/dbt-deps' of github.com:anjutiwari/dbt-core int…
gshank Aug 8, 2023
abb6c24
fix formatting of changelog
gshank Aug 8, 2023
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
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20230424-210734.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Copy dir during `dbt deps` if symlink fails
time: 2023-04-24T21:07:34.336797+05:30
custom:
Author: anjutiwari
Issue: "7428 8223"
8 changes: 2 additions & 6 deletions core/dbt/deps/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,15 @@ def install(self, project, renderer):
src_path = self.resolve_path(project)
dest_path = self.get_installation_path(project, renderer)

can_create_symlink = system.supports_symlinks()

if system.path_exists(dest_path):
if not system.path_is_symlink(dest_path):
system.rmdir(dest_path)
else:
system.remove_file(dest_path)

if can_create_symlink:
Copy link

Choose a reason for hiding this comment

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

if can_create_symlink check shouldn't be removed. It helps for the cases where symlink function doesn't exist on os, in which case AttributeError would be thrown.

except OSError should be added as an edge case like ours, where os.symlink function exists but it still fails.

Something like:

symlink_supported = False

if can_create_symlink:
    try:
        fire_event(DepsCreatingLocalSymlink())
        system.make_symlink(src_path, dest_path)
    except OSError:
        pass
    else:
        symlink_supported = True
if not symlink_supported:
    fire_event(DepsSymlinkNotAvailable())
    shutil.copytree(src_path, dest_path)

Copy link
Contributor Author

@anjutiwari anjutiwari Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think we need it, make_symlink calling it. The functionality remains the same. Check make_symlink method.
https://github.com/anjutiwari/dbt-core/blob/main/core/dbt/clients/system.py#L156
https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md

Copy link

Choose a reason for hiding this comment

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

make_symlink method will throw dbt.exceptions.SymbolicLinkError, in which case except OSError block here won't handle it.
The behaviour is changed for the case where symlink function attribute isn't present on os

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we should use dbt.exceptions.SymbolicLinkError, and we should also catch the SymbolicLinkError.

Also, I am curious about which OS does not have symlinks function in python lib(os) package, as mentioned in my Issue description.
As per the dbt, we have a list of supported operating systems and condition on python3.7+. https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md

If my understanding is true, we may not have any scenarios where lib(os) package will not have a symlink function and we may not need to handle the SymbolicLinkError exception.

Waiting for the core reviewers to help us here.

try:
fire_event(DepsCreatingLocalSymlink())
system.make_symlink(src_path, dest_path)

else:
except OSError:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.getdbt.com/faqs/core/install-pip-os-prereqs.md
Handling exception case of DBFS

fire_event(DepsSymlinkNotAvailable())
shutil.copytree(src_path, dest_path)

Expand Down
17 changes: 16 additions & 1 deletion tests/unit/test_deps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import dbt.deps
import dbt.exceptions
from dbt.deps.git import GitUnpinnedPackage
from dbt.deps.local import LocalUnpinnedPackage
from dbt.deps.local import LocalUnpinnedPackage, LocalPinnedPackage
from dbt.deps.tarball import TarballUnpinnedPackage
from dbt.deps.registry import RegistryUnpinnedPackage
from dbt.clients.registry import is_compatible_version
Expand Down Expand Up @@ -92,6 +92,21 @@ def test_init(self):
self.assertEqual(a_pinned.source_type(), "git")
self.assertIs(a_pinned.warn_unpinned, True)

@mock.patch("shutil.copytree")
@mock.patch("dbt.deps.local.system.make_symlink")
@mock.patch("dbt.deps.local.LocalPinnedPackage.get_installation_path")
@mock.patch("dbt.deps.local.LocalPinnedPackage.resolve_path")
def test_deps_install(
self, mock_resolve_path, mock_get_installation_path, mock_symlink, mock_shutil
):
mock_resolve_path.return_value = "/tmp/source"
mock_get_installation_path.return_value = "/tmp/dest"
mock_symlink.side_effect = OSError("Install deps symlink error")

LocalPinnedPackage("local").install("dummy", "dummy")
self.assertEqual(mock_shutil.call_count, 1)
mock_shutil.assert_called_once_with("/tmp/source", "/tmp/dest")

def test_invalid(self):
with self.assertRaises(ValidationError):
GitPackage.validate(
Expand Down
Loading