fix: fix git dependencies comparison on bootstrap #659

mateusfccp · 2024-03-13T18:35:10Z

Description

Fixes comparison check of GitReference by also comparing the path.

Fixes #658.

Type of Change

✨ feat -- New feature (non-breaking change which adds functionality)
🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
❌ ! -- Breaking change (fix or feature that would cause existing functionality to change)
🧹 refactor -- Code refactor
✅ ci -- Build configuration change
📝 docs -- Documentation
🗑️ chore -- Chore

CLAassistant · 2024-03-13T18:35:16Z

All committers have signed the CLA.

spydon

Can you add a very simple test that verifies this, so that we don't get a regression later?

packages/melos/lib/src/commands/bootstrap.dart

mateusfccp · 2024-03-14T16:58:03Z

Can you add a very simple test that verifies this, so that we don't get a regression later?

Sure, but it will take a little longer, as I'm not very acquainted with this repository's tests.

spydon · 2024-03-14T17:09:58Z

Sure, but it will take a little longer, as I'm not very acquainted with this repository's tests.

Super! You've probably already found it, but if not, you might be able to get some inspiration from the existing bootstrap tests (where the new test should live too):
https://github.com/invertase/melos/blob/main/packages/melos/test/commands/bootstrap_test.dart

mateusfccp · 2024-03-20T17:05:13Z

Hey, @spydon.

Do you have any idea on how can I test git cloning with the tests suit? For path, for instance, we have createTestTempDir, so that we can create temporary files that we use for testing.

For git, however, we have nothing. I don't fell like it would be advisable to test with a real repository, so I think we should fake this process, but I am not aware how we could do it.

Currently, I'm using this fake object for a git reference:

final pkg = await createProject(
  workspaceDir,
  const PubSpec(
    name: 'package',
    dependencies: {
      'git': GitReference(
        'url',
        'reference',
        'path',
      ),
    },
  ),
);

However, as I'm not faking the git repository, melos test tries to clone from bare repository /Users/<myuser>/.puro/shared/pub_cache/git/cache/url-0a6bd4f3b43c4001eceeb17e5638198fa98773db and fails miserably.

spydon · 2024-03-20T17:11:48Z

Hey again @mateusfccp!

Do you have any idea on how can I test git cloning with the tests suit? For path, for instance, we have createTestTempDir, so that we can create temporary files that we use for testing.

Can't you do the same, but with a temporary local git repository? You should be able to use a git file path url (not a path dependency) to that temporary repository.

mateusfccp · 2024-03-29T00:18:08Z

@spydon

Can't you do the same, but with a temporary local git repository?

I actually didn't know we could do something like this. It worked!

I'm not sure if this is the most straightforward way to test this, but it works. Let me know if I can improve it somehow.

spydon

Lgtm, I think that test is good enough

mateusfccp · 2024-03-29T00:40:45Z

It seems it failed on Windows and Linux, although it worked on Mac (the platform I'm working with).

The error seems to indicate that the path is not a git repository, even tho I initialized it and made a commit.

Unfortunately, I have no Linux or Window machine to debug the problem here... Do you have any idea what it could be?

Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>

spydon · 2024-04-01T14:24:43Z

It seems it failed on Windows and Linux, although it worked on Mac (the platform I'm working with).

The error seems to indicate that the path is not a git repository, even tho I initialized it and made a commit.

Unfortunately, I have no Linux or Window machine to debug the problem here... Do you have any idea what it could be?

Sorry, I though I replied after reading it a few days ago!
Seems like you managed to sort it out?

mateusfccp · 2024-04-01T16:55:27Z

It seems it failed on Windows and Linux, although it worked on Mac (the platform I'm working with).
The error seems to indicate that the path is not a git repository, even tho I initialized it and made a commit.
Unfortunately, I have no Linux or Window machine to debug the problem here... Do you have any idea what it could be?

Sorry, I though I replied after reading it a few days ago! Seems like you managed to sort it out?

No, not really. I just rebased the PR...

I searched for potential differences in git between platforms, but didn't find anything relevant, so still in the darkness.

Edit:
I just asked for a friend to try to run the test in his Windows machine and the test passed.

Maybe it's some difference in the CI environment? Could be permission related?

spydon · 2024-04-01T21:37:08Z

@mateusfccp just realized you probably can't see the CI logs?
Here's what it says:

   Resolving dependencies...
  -Because git_references depends on dependency from git which doesn't exist (Could not find git ref 'HEAD' (usage: git rev-list [<options>] <commit>... [--] [<path>...]
  -  limiting output:
...
  -    --bisect-all)), version solving failed.
  
  BootstrapException: Failed to install.: git_references at /home/runner/work/melos/melos/packages/melos/.dart_tool/GKWRBX/packages/git_references.
  package:melos/src/commands/bootstrap.dart 243:7  _BootstrapMixin._runPubGetForPackage

mateusfccp · 2024-04-01T22:47:41Z

@mateusfccp just realized you probably can't see the CI logs?
Here's what it says:

   Resolving dependencies...
  -Because git_references depends on dependency from git which doesn't exist (Could not find git ref 'HEAD' (usage: git rev-list [<options>] <commit>... [--] [<path>...]
  -  limiting output:
...
  -    --bisect-all)), version solving failed.
  
  BootstrapException: Failed to install.: git_references at /home/runner/work/melos/melos/packages/melos/.dart_tool/GKWRBX/packages/git_references.
  package:melos/src/commands/bootstrap.dart 243:7  _BootstrapMixin._runPubGetForPackage

Yes, I can see them. Like I said, this error seems to mean that the git repository was not created correctly or the commit.

I did pass through this error while writing the test, because at first I was only creating the git repository with git init, but I realized that to have a valid reference you have to have at least one commit, this is why I added the initial commit to the test.

This is why I think either the git initialization or the commit is failing in some weird way.

spydon · 2024-04-02T09:19:43Z

packages/melos/test/commands/bootstrap_test.dart

+
+      await io.Process.run(
+        'git',
+        ['add', '-A'],


Empty directories won't be added to git, maybe create an empty file in both of the directories first?

The directory, supposedly, is not empty, as we created two projects within it with createProject.

If this was not the case, the test would not pass in my local machine either.

Ah right, I read createDirectory and not createProject when I read it this morning. 😅
The test passes locally for me too (linux), so permissions does sound like a good guess, but it doesn't look like it from the error message in the pipeline, if it doesn't fail silently earlier.

I am going to try to reproduce the problem by using a container locally and running the same images. Maybe I can debug this way.

Good idea, I re-ran the failing test with GitHub debug printing, but it didn't give anything

@mateusfccp did you manage to find anything causing it?

Hello, @spydon.

Sorry for the delay. I actually didn't get to reproduce the environment on my local machine. I tried to do it with act, but with no success.

mateusfccp force-pushed the main branch from 5c3a6aa to 1cdca1c Compare March 13, 2024 18:38

spydon reviewed Mar 14, 2024

View reviewed changes

packages/melos/lib/src/commands/bootstrap.dart Show resolved Hide resolved

mateusfccp force-pushed the main branch from d3ea270 to 21c6b38 Compare March 29, 2024 00:16

spydon approved these changes Mar 29, 2024

View reviewed changes

mateusfccp added 3 commits April 1, 2024 09:09

fix: fix git dependencies comparison on bootstrap

ef8399b

Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>

Add todo to code

11a72eb

Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>

add test to behavior fix

c03ce6a

Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>

mateusfccp force-pushed the main branch from 21c6b38 to c03ce6a Compare April 1, 2024 12:09

spydon reviewed Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix git dependencies comparison on bootstrap #659

fix: fix git dependencies comparison on bootstrap #659

mateusfccp commented Mar 13, 2024

CLAassistant commented Mar 13, 2024 •

edited

Loading

spydon left a comment

mateusfccp commented Mar 14, 2024

spydon commented Mar 14, 2024

mateusfccp commented Mar 20, 2024

spydon commented Mar 20, 2024

mateusfccp commented Mar 29, 2024 •

edited

Loading

spydon left a comment

mateusfccp commented Mar 29, 2024

spydon commented Apr 1, 2024

mateusfccp commented Apr 1, 2024 •

edited

Loading

spydon commented Apr 1, 2024

mateusfccp commented Apr 1, 2024

spydon Apr 2, 2024

mateusfccp Apr 2, 2024 •

edited

Loading

spydon Apr 2, 2024

mateusfccp Apr 5, 2024

spydon Apr 5, 2024

spydon Jun 5, 2024

mateusfccp Jun 5, 2024

fix: fix git dependencies comparison on bootstrap #659

Are you sure you want to change the base?

fix: fix git dependencies comparison on bootstrap #659

Conversation

mateusfccp commented Mar 13, 2024

Description

Type of Change

CLAassistant commented Mar 13, 2024 • edited Loading

spydon left a comment

Choose a reason for hiding this comment

mateusfccp commented Mar 14, 2024

spydon commented Mar 14, 2024

mateusfccp commented Mar 20, 2024

spydon commented Mar 20, 2024

mateusfccp commented Mar 29, 2024 • edited Loading

spydon left a comment

Choose a reason for hiding this comment

mateusfccp commented Mar 29, 2024

spydon commented Apr 1, 2024

mateusfccp commented Apr 1, 2024 • edited Loading

spydon commented Apr 1, 2024

mateusfccp commented Apr 1, 2024

spydon Apr 2, 2024

Choose a reason for hiding this comment

mateusfccp Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

spydon Apr 2, 2024

Choose a reason for hiding this comment

mateusfccp Apr 5, 2024

Choose a reason for hiding this comment

spydon Apr 5, 2024

Choose a reason for hiding this comment

spydon Jun 5, 2024

Choose a reason for hiding this comment

mateusfccp Jun 5, 2024

Choose a reason for hiding this comment

CLAassistant commented Mar 13, 2024 •

edited

Loading

mateusfccp commented Mar 29, 2024 •

edited

Loading

mateusfccp commented Apr 1, 2024 •

edited

Loading

mateusfccp Apr 2, 2024 •

edited

Loading