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

(partially) fix issue #6888, source-repository-package branch failure #9154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

noinia
Copy link

@noinia noinia commented Jul 27, 2023

As the title of the issue states, this (partially) fixes issue #6888 ; i.e. if a user specifies a git branch in the 'source-repository-package' stanza in cabal.project cabal actually properly checks out that branch.

The core of the issue is that (1) the (existing) implementation always uses 'vcsSyncRepos' to check out a (git) repository rather than 'vcsCloneRepo', and (2) vcsSyncRepos did not specify the '--branch' field to clone. (Afterwards it also runs git reset --hard ', but that fails since it did not clone the right initial branch.

This PR provides a somewhat minimalistic approach to solving this issue: i.e. it now also provides the branch to clone in the implementation of vcsSyncRepos in vcsGit, which fixes the issue.

I do think this reveals some opportunity for more refactoring though. In particular it seems that

  • vcsCloneRepo is not actually used anywhere in the project !?
  • I only added the --branch stuff to the implementation of vcsGit; I would think that the other vcs systems (hg, bazaar etc) probably also need some sort of --branch argument in order to work correctly. Just glancing at the code there I'm guessing that does not work either.

Not entirely sure what to do about these two remaining parts. I can't easily figure out/test what the right --branch invocations would be for the other VCSes. If desired, ripping out vcsCloneRepo, is probably done best in a separate PR. I'm happy to hear input about this. (Afterwards I can write the changelog entry).

**
Include the following checklist in your PR:

@andreabedini
Copy link
Collaborator

I do think this reveals some opportunity for more refactoring though.

YES. Please make yourself at home :) How would you write that code?

vcsCloneRepo is called by cloneSourceRepo.

@noinia
Copy link
Author

noinia commented Jul 27, 2023

I do think this reveals some opportunity for more refactoring though.

YES. Please make yourself at home :) How would you write that code?

Good question. Admittedly, I don't have a concrete proposal; but it seems strange to me that: vcsCloneRepo does not seem to be called at all when cloning a new git repository, and that vcsSyncRepo duplicates some of the cloning logic.

vcsCloneRepo is called by cloneSourceRepo.

Hmm, well spotted. Still, it seems that even on a fresh clone vcsSyncRepo is used rater than vcsCloneRepo. So I don't think vcsCloneRepo is actually invoked when cloning a fresh repository.

@noinia
Copy link
Author

noinia commented Jul 27, 2023

Hmm, for whatever reason CI seems to timeout with ghc 9.0.2 on OS X. Not sure why though.

@soulomoon
Copy link

How is the state of it.

@noinia
Copy link
Author

noinia commented Apr 11, 2024

Patch seems to work fine here; I've been using it locally ever since opening this PR.

I don't have the time and interest to do any (further) refactoring steps that may or may not be possible here. In any case: there has not been much input from the maintainers on whether they want this patch/solution and/or what else is required (other than me writing a changelog entry I guess)

@noinia
Copy link
Author

noinia commented Sep 18, 2024

I now also wrote the changelog entry, so this should really be done now.

The failure on macos-13 ghc-9.10.1 is because there was some (temporary?) internet issue it seems. Not sure if I can easily restart just that runner; I didn't really see a way to do that. (But I don't really have any reason to believe it would not sucessfully build there otherwise; I'm locally using mac os and 9.10.1 at the moment even actually).

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.

3 participants