bitbake-devel.lists.openembedded.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs
@ 2023-02-17 17:00 Paulo Neves
  2023-02-17 17:00 ` [PATCH 2/5] test: Add real git lfs tests and decorator Paulo Neves
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Paulo Neves @ 2023-02-17 17:00 UTC (permalink / raw)
  To: bitbake-devel; +Cc: Paulo Neves

Not restoring the mocked _find_git_lfs leads to other tests
failing.

Signed-off-by: Paulo Neves <paulo@myneves.com>
---
 lib/bb/tests/fetch.py | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index f3890321d..6d16cf59a 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -2250,12 +2250,16 @@ class GitLfsTest(FetcherTest):
             shutil.rmtree(self.gitdir, ignore_errors=True)
             fetcher.unpack(self.d.getVar('WORKDIR'))

-        # If git-lfs cannot be found, the unpack should throw an error
-        with self.assertRaises(bb.fetch2.FetchError):
-            fetcher.download()
-            ud.method._find_git_lfs = lambda d: False
-            shutil.rmtree(self.gitdir, ignore_errors=True)
-            fetcher.unpack(self.d.getVar('WORKDIR'))
+        old_find_git_lfs = ud.method._find_git_lfs
+        try:
+            # If git-lfs cannot be found, the unpack should throw an error
+            with self.assertRaises(bb.fetch2.FetchError):
+                fetcher.download()
+                ud.method._find_git_lfs = lambda d: False
+                shutil.rmtree(self.gitdir, ignore_errors=True)
+                fetcher.unpack(self.d.getVar('WORKDIR'))
+        finally:
+            ud.method._find_git_lfs = old_find_git_lfs

     def test_lfs_disabled(self):
         import shutil
@@ -2270,17 +2274,21 @@ class GitLfsTest(FetcherTest):
         fetcher, ud = self.fetch()
         self.assertIsNotNone(ud.method._find_git_lfs)

-        # If git-lfs can be found, the unpack should be successful. A
-        # live copy of git-lfs is not required for this case, so
-        # unconditionally forge its presence.
-        ud.method._find_git_lfs = lambda d: True
-        shutil.rmtree(self.gitdir, ignore_errors=True)
-        fetcher.unpack(self.d.getVar('WORKDIR'))
+        old_find_git_lfs = ud.method._find_git_lfs
+        try:
+            # If git-lfs can be found, the unpack should be successful. A
+            # live copy of git-lfs is not required for this case, so
+            # unconditionally forge its presence.
+            ud.method._find_git_lfs = lambda d: True
+            shutil.rmtree(self.gitdir, ignore_errors=True)
+            fetcher.unpack(self.d.getVar('WORKDIR'))
+            # If git-lfs cannot be found, the unpack should be successful

-        # If git-lfs cannot be found, the unpack should be successful
-        ud.method._find_git_lfs = lambda d: False
-        shutil.rmtree(self.gitdir, ignore_errors=True)
-        fetcher.unpack(self.d.getVar('WORKDIR'))
+            ud.method._find_git_lfs = lambda d: False
+            shutil.rmtree(self.gitdir, ignore_errors=True)
+            fetcher.unpack(self.d.getVar('WORKDIR'))
+        finally:
+            ud.method._find_git_lfs = old_find_git_lfs

 class GitURLWithSpacesTest(FetcherTest):
     test_git_urls = {
--
2.34.1




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/5] test: Add real git lfs tests and decorator
  2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
@ 2023-02-17 17:00 ` Paulo Neves
  2023-02-17 17:00 ` [PATCH 3/5] git: Remove is None check on _find_git_lfs Paulo Neves
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Paulo Neves @ 2023-02-17 17:00 UTC (permalink / raw)
  To: bitbake-devel; +Cc: Paulo Neves

Added tests that verify that git-lfs works with an actual
real git-lfs server. This was not previously the case because
the repo in the test was a simulation of git-lfs but not
a real git lfs repo.

The 2 added tests are almost the same but test that the
git lfs file checkout is successfult with or without the
lfs=1 flag. The lfs=1 URI parameter is a quirk that triggers
2 different code paths for git lfs.

lfs=1, when used on git lfs repositories triggers the git lfs
downloading at the fetch bare stage.

lfs query parameter unset triggers the git lfs downloading only
on checkout as an implicit behavior of git. This leads to possible
network access on the unpack stage and outside the DL_DIR.

lfs=0 actually disables git-lfs functionality even if supported.

Signed-off-by: Paulo Neves <paulo@myneves.com>
---
 lib/bb/tests/fetch.py | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 6d16cf59a..54148b3fd 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -2199,6 +2199,12 @@ class GitShallowTest(FetcherTest):
         self.assertIn("fstests.doap", dir)

 class GitLfsTest(FetcherTest):
+    def skipIfNoGitLFS():
+        import shutil
+        if not shutil.which('git-lfs'):
+            return unittest.skip('git-lfs not installed')
+        return lambda f: f
+
     def setUp(self):
         FetcherTest.setUp(self)

@@ -2232,6 +2238,44 @@ class GitLfsTest(FetcherTest):
         ud = fetcher.ud[uri]
         return fetcher, ud

+    def get_real_git_lfs_file(self):
+        self.d.setVar('PATH', os.environ.get('PATH'))
+        fetcher, ud = self.fetch()
+        fetcher.unpack(self.d.getVar('WORKDIR'))
+        unpacked_lfs_file = os.path.join(self.d.getVar('WORKDIR'), 'git', "Cat_poster_1.jpg")
+        return unpacked_lfs_file
+
+    @skipIfNoGitLFS()
+    @skipIfNoNetwork()
+    def test_real_git_lfs_repo_succeeds_without_lfs_param(self):
+        self.d.setVar('SRC_URI', "git://gitlab.com/gitlab-examples/lfs.git;protocol=https;branch=master")
+        f = self.get_real_git_lfs_file()
+        self.assertTrue(os.path.exists(f))
+        self.assertEqual("c0baab607a97839c9a328b4310713307", bb.utils.md5_file(f))
+
+    @skipIfNoGitLFS()
+    @skipIfNoNetwork()
+    def test_real_git_lfs_repo_succeeds(self):
+        self.d.setVar('SRC_URI', "git://gitlab.com/gitlab-examples/lfs.git;protocol=https;branch=master;lfs=1")
+        f = self.get_real_git_lfs_file()
+        self.assertTrue(os.path.exists(f))
+        self.assertEqual("c0baab607a97839c9a328b4310713307", bb.utils.md5_file(f))
+
+    @skipIfNoGitLFS()
+    @skipIfNoNetwork()
+    def test_real_git_lfs_repo_succeeds(self):
+        self.d.setVar('SRC_URI', "git://gitlab.com/gitlab-examples/lfs.git;protocol=https;branch=master;lfs=0")
+        f = self.get_real_git_lfs_file()
+        # This is the actual non-smudged placeholder file on the repo if git-lfs does not run
+        lfs_file = (
+                   'version https://git-lfs.github.com/spec/v1\n'
+                   'oid sha256:34be66b1a39a1955b46a12588df9d5f6fc1da790e05cf01f3c7422f4bbbdc26b\n'
+                   'size 11423554\n'
+        )
+
+        with open(f) as fh:
+            self.assertEqual(lfs_file, fh.read())
+
     def test_lfs_enabled(self):
         import shutil

--
2.34.1




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/5] git: Remove is None check on  _find_git_lfs
  2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
  2023-02-17 17:00 ` [PATCH 2/5] test: Add real git lfs tests and decorator Paulo Neves
@ 2023-02-17 17:00 ` Paulo Neves
  2023-02-22 12:03   ` [bitbake-devel] " Richard Purdie
  2023-02-17 17:01 ` [PATCH 4/5] git.py: @_contains_lfs: Removed unused variables Paulo Neves
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Paulo Neves @ 2023-02-17 17:00 UTC (permalink / raw)
  To: bitbake-devel; +Cc: Paulo Neves

shutil.which returns None when the argument is not found.
As per documentation[1] None is evaluated to False and everything
else is True, so it is safe to have _find_git_lfs just return
the value of shutil.which

[1] https://docs.python.org/3/library/stdtypes.html#truth-value-testing

Signed-off-by: Paulo Neves <paulo@myneves.com>
---
 lib/bb/fetch2/git.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 2e3d32515..a3f29d2b9 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -687,7 +687,7 @@ class Git(FetchMethod):
         Return True if git-lfs can be found, False otherwise.
         """
         import shutil
-        return shutil.which("git-lfs", path=d.getVar('PATH')) is not None
+        return shutil.which("git-lfs", path=d.getVar('PATH'))

     def _get_repo_url(self, ud):
         """
--
2.34.1




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/5] git.py: @_contains_lfs: Removed unused variables
  2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
  2023-02-17 17:00 ` [PATCH 2/5] test: Add real git lfs tests and decorator Paulo Neves
  2023-02-17 17:00 ` [PATCH 3/5] git: Remove is None check on _find_git_lfs Paulo Neves
@ 2023-02-17 17:01 ` Paulo Neves
  2023-02-17 17:01 ` [PATCH 5/5] git.py Replace mkdtemp with TemporaryDirectory and avoid exception masking Paulo Neves
  2023-02-22 12:01 ` [bitbake-devel] [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Richard Purdie
  4 siblings, 0 replies; 7+ messages in thread
From: Paulo Neves @ 2023-02-17 17:01 UTC (permalink / raw)
  To: bitbake-devel; +Cc: Paulo Neves

branchname was set but never used in the context of _contains_lfs
method.

Signed-off-by: Paulo Neves <paulo@myneves.com>
---
 lib/bb/fetch2/git.py | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index a3f29d2b9..fcd70fdcf 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -660,11 +660,6 @@ class Git(FetchMethod):
         Check if the repository has 'lfs' (large file) content
         """

-        if not ud.nobranch:
-            branchname = ud.branches[ud.names[0]]
-        else:
-            branchname = "master"
-
         # The bare clonedir doesn't use the remote names; it has the branch immediately.
         if wd == ud.clonedir:
             refname = ud.branches[ud.names[0]]
--
2.34.1




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 5/5] git.py Replace mkdtemp with TemporaryDirectory and avoid exception masking
  2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
                   ` (2 preceding siblings ...)
  2023-02-17 17:01 ` [PATCH 4/5] git.py: @_contains_lfs: Removed unused variables Paulo Neves
@ 2023-02-17 17:01 ` Paulo Neves
  2023-02-22 12:01 ` [bitbake-devel] [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Richard Purdie
  4 siblings, 0 replies; 7+ messages in thread
From: Paulo Neves @ 2023-02-17 17:01 UTC (permalink / raw)
  To: bitbake-devel; +Cc: Paulo Neves

Due to using mkdtemp instead of TemporaryDirectory we needed to
manually cleanup the directory in a try finally block.
With tempfile.TemporaryDirectory we can handle the cleanup with
a "with" statement and not need to manually clean up oursevels.

Signed-off-by: Paulo Neves <paulo@myneves.com>
---
 lib/bb/fetch2/git.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index fcd70fdcf..9445643e5 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -417,8 +417,7 @@ class Git(FetchMethod):
             # It would be nice to just do this inline here by running 'git-lfs fetch'
             # on the bare clonedir, but that operation requires a working copy on some
             # releases of Git LFS.
-            tmpdir = tempfile.mkdtemp(dir=d.getVar('DL_DIR'))
-            try:
+            with tempfile.TemporaryDirectory(dir=d.getVar('DL_DIR')) as tmpdir:
                 # Do the checkout. This implicitly involves a Git LFS fetch.
                 Git.unpack(self, ud, tmpdir, d)

@@ -436,8 +435,6 @@ class Git(FetchMethod):
                 # downloaded.
                 if os.path.exists(os.path.join(tmpdir, "git", ".git", "lfs")):
                     runfetchcmd("tar -cf - lfs | tar -xf - -C %s" % ud.clonedir, d, workdir="%s/git/.git" % tmpdir)
-            finally:
-                bb.utils.remove(tmpdir, recurse=True)

     def build_mirror_data(self, ud, d):

--
2.34.1




^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [bitbake-devel] [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs
  2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
                   ` (3 preceding siblings ...)
  2023-02-17 17:01 ` [PATCH 5/5] git.py Replace mkdtemp with TemporaryDirectory and avoid exception masking Paulo Neves
@ 2023-02-22 12:01 ` Richard Purdie
  4 siblings, 0 replies; 7+ messages in thread
From: Richard Purdie @ 2023-02-22 12:01 UTC (permalink / raw)
  To: Paulo Neves, bitbake-devel

On Fri, 2023-02-17 at 17:00 +0000, Paulo Neves wrote:
> Not restoring the mocked _find_git_lfs leads to other tests
> failing.
> 
> Signed-off-by: Paulo Neves <paulo@myneves.com>
> ---
>  lib/bb/tests/fetch.py | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)

Just for future reference, the shortlog entries for these patches needs
tweaking. For example in this case I'd have written:

tests/fetch: git-lfs restore _find_git_lfs

since that makes it clear it is the fetch test being changed.

In other changes you used both "git" and "git.py", I'd suggest just
using "git" and I'd probably use "fetch/git" to make it clear it was
the git fetcher.

I'll fix up these in the patch queue this time so no need to resend.

Cheers,

Richard



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [bitbake-devel] [PATCH 3/5] git: Remove is None check on _find_git_lfs
  2023-02-17 17:00 ` [PATCH 3/5] git: Remove is None check on _find_git_lfs Paulo Neves
@ 2023-02-22 12:03   ` Richard Purdie
  0 siblings, 0 replies; 7+ messages in thread
From: Richard Purdie @ 2023-02-22 12:03 UTC (permalink / raw)
  To: Paulo Neves, bitbake-devel

On Fri, 2023-02-17 at 17:00 +0000, Paulo Neves wrote:
> shutil.which returns None when the argument is not found.
> As per documentation[1] None is evaluated to False and everything
> else is True, so it is safe to have _find_git_lfs just return
> the value of shutil.which
> 
> [1] https://docs.python.org/3/library/stdtypes.html#truth-value-testing
> 
> Signed-off-by: Paulo Neves <paulo@myneves.com>
> ---
>  lib/bb/fetch2/git.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
> index 2e3d32515..a3f29d2b9 100644
> --- a/lib/bb/fetch2/git.py
> +++ b/lib/bb/fetch2/git.py
> @@ -687,7 +687,7 @@ class Git(FetchMethod):
>          Return True if git-lfs can be found, False otherwise.
>          """
>          import shutil
> -        return shutil.which("git-lfs", path=d.getVar('PATH')) is not None
> +        return shutil.which("git-lfs", path=d.getVar('PATH'))
> 
>      def _get_repo_url(self, ud):
>          """

I don't agree with this change since currently the function does return
True/False as it says in the docstring but after your change, it
returns other things.

Yes, evaluating these in python will result in True/False and that is
safe but that isn't what it is documented to return.

Cheers,

Richard


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-02-22 12:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-17 17:00 [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Paulo Neves
2023-02-17 17:00 ` [PATCH 2/5] test: Add real git lfs tests and decorator Paulo Neves
2023-02-17 17:00 ` [PATCH 3/5] git: Remove is None check on _find_git_lfs Paulo Neves
2023-02-22 12:03   ` [bitbake-devel] " Richard Purdie
2023-02-17 17:01 ` [PATCH 4/5] git.py: @_contains_lfs: Removed unused variables Paulo Neves
2023-02-17 17:01 ` [PATCH 5/5] git.py Replace mkdtemp with TemporaryDirectory and avoid exception masking Paulo Neves
2023-02-22 12:01 ` [bitbake-devel] [PATCH 1/5] tests: git-lfs: Restore _find_git_lfs Richard Purdie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).