buildinspace / peru

a generic package manager, for including other people's code in your projects
MIT License
1.12k stars 69 forks source link

Bug: reup considers master as default branch #213

Closed felipefoz closed 3 years ago

felipefoz commented 3 years ago

I found a bug when trying to use peru with reup, it returned an error when trying to reach the master branch. My code has a default branch called main, as right now, some repositories are using by default.

peru/resources/plugins/git/git_plugin.py

def main():
    URL = os.environ['PERU_MODULE_URL']
    REV = os.environ['PERU_MODULE_REV'] or 'master'
    REUP = os.environ['PERU_MODULE_REUP'] or 'master'

I found this code and I think it is the culprit.

A solution could be a function to return the default branch instead of assuming its master? There are many solutions for this problem, we might need to find one fast and reliable.

oconnor663 commented 3 years ago

Yes I've run into this myself. To work around this in an individual case, currently you can use the optional reup field:

git module foo:
  url: https://github.com/foo/foo
  rev: f000000000000000000000000000000000000000
  reup: main

Alternatively, if you always fetch the latest commit without specifying a hash (not usually the recommended approach, but there are valid use cases), you can use the rev field itself:

git module foo:
  url: https://github.com/foo/foo
  rev: main

Going forward, I think we can find a way to make those workarounds unnecessary. For example, after we clone the remote repo, we could just inspect it to see if either master or main is present, so if e.g. only main is present then we could have rev and reup default to main. I bet that will work, but I haven't tried it yet. (Making this change in peru/resources/plugins/git/git_plugin.py should be sufficient. I don't think the core peru code needs to know about this git-specific detail.)

felipefoz commented 3 years ago

Great! Good to know there is a workaround by now. I tried modifying this part of the code to main and it worked.

I thought to include a function (see below) returning a string to replace that hardcorded master there. Your idea would be to insert after the clone?

Here is a long discussion with plenty of methods for getting the default branch: https://stackoverflow.com/questions/28666357/git-how-to-get-default-branch

felipefoz commented 3 years ago

Something like this works, what do you think?

def git_default_branch(url):
    """
    This function returns the default branch of the git.
    By default git used to take master as default branch, but newer versions tend to use main.
    Other default branches such as trunk also might be found.
    """ 
    repo_path = repo_cache_path(url)
    output = git('symbolic-ref', '--short','HEAD', git_dir=repo_path, capture_output=True).output
    default_branch = output.strip()
    return default_branch

def main():
    URL = os.environ['PERU_MODULE_URL']
    REV = os.environ['PERU_MODULE_REV'] or git_default_branch(URL)
    REUP = os.environ['PERU_MODULE_REUP'] or git_default_branch(URL)

Not sure about testing, how it would fit there, any help?

oconnor663 commented 3 years ago

I'm pretty sure you'll need to call clone_if_needed, but this is on the right track.

For testing, look for this test in test_plugins.py:

    def test_git_plugin(self):
        GitRepo(self.content_dir)
        self.do_plugin_test("git", {"url": self.content_dir}, self.content)

We could write another tiny test like that one, against a repo whose default branch is main. We'd want to add a keyword argument to the GitRepo helper class to specify this.

felipefoz commented 3 years ago

Solved with Merge Request #214.