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: fix git dependencies comparison on bootstrap #659

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mateusfccp
Copy link

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
Copy link

CLAassistant commented Mar 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

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

@mateusfccp
Copy link
Author

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
Copy link
Collaborator

spydon commented Mar 14, 2024

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
Copy link
Author

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
Copy link
Collaborator

spydon commented Mar 20, 2024

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
Copy link
Author

mateusfccp commented Mar 29, 2024

@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.

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, I think that test is good enough

@mateusfccp
Copy link
Author

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]>
Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>
Signed-off-by: Mateus Felipe C. C. Pinto <[email protected]>
@spydon
Copy link
Collaborator

spydon commented Apr 1, 2024

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
Copy link
Author

mateusfccp commented Apr 1, 2024

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.

image

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

@spydon
Copy link
Collaborator

spydon commented Apr 1, 2024

@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
Copy link
Author

@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.


await io.Process.run(
'git',
['add', '-A'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

@mateusfccp mateusfccp Apr 2, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mateusfccp did you manage to find anything causing it?

Copy link
Author

Choose a reason for hiding this comment

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

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.

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.

melos bootstrap should consider path when comparing two GitReferences
3 participants