-
Notifications
You must be signed in to change notification settings - Fork 56
Add support for git repos with lfs #259
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
Add support for git repos with lfs #259
Conversation
micprog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this addition, I think it makes sense and is very useful in case lfs is enabled and needed!
Unfortunately, it will fail if git-lfs is not installed, which may be the case. Maybe we can disable the lfs functionality with the following:
- if there is a configuration in a bender configuration file (e.g.,
Bender.local) disabling this. This can be useful if the lfs files are known to not be required. - if the repository doesn't require it. This way we save running the command and keep bender working for systems that don't currently have git-lfs installed.
- Maybe we can add a warning if it required but not supported (or disabled) instead of erroring out on the lfs comands completely.
My main concern is ensuring the existing systems and functionality do not break by adding this new feature. Happy to help out to make the required modifications.
I also see you added a file (bender-lfs-debug-top), I don't believe this is needed.
205599d to
d42a4c6
Compare
|
Added functionality per points 2 and 3 to check for system install of git-lfs and also a warning if not supported but required. Also should only run if the repo has lfs in .gitattribute. Not sure how to structure a solution for the first point - should it be for the local repository or for the entire dependency tree? |
micprog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, they look great!
One question you might know more about - can the LFS URL be configured by default to not point to the normal git URL, with this information already present in the checked-out repo? If so, we may need to ensure we don't overwrite an existing configuration. If this is not possible, then I think we are OK, as the URL points to the git URL by default. I'm not very familiar with LFS, and GitHub has strict limits on it, so we do not use it for our open-source projects.
Other than that, I added some minor style changes and enabled checking for .gitattributes before checking for git-lfs on top of your changes. I also added global disabling of git-lfs with a configuration. Unfortunately I cannot push to your branch, but the changes are in the main repository: https://github.com/pulp-platform/bender/tree/bugfix/pulp_lfs_fix
If you are OK with the additional updates I added, I would be grateful if you could add them to your branch so I can merge them in one go!
|
I pulled your changes into the PR branch. I am also not a deep expert in the variations possible with LFS - we just use it to store large things like liberty files with the benefit of revision control so our configurations are fairly vanilla. But since we have vendor releases that we want to minimally Bender-ize without splitting out large synthesis and physical design artifacts, we chose the LFS route. Thanks for your support in considering this. |
Fixes #258
Modified src/sess.rs
Disable LFS smudging during the internal git clone / git checkout operation from the DB repo to the workspace checkout directory. This allows the checkout to succeed even with missing LFS objects. After checkout, configure lfs.url in the checkout directory to point to the original upstream repository URL.
Run git lfs pull to explicitly download the LFS objects from the upstream.