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

introducing modern conan.tool.scm.Git helper #10594

Merged
merged 41 commits into from
Mar 2, 2022

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Feb 16, 2022

Changelog: Feature: Introduce new conan.tools.scm.Git helper, for direct use in export() method to capture git url and commit, and to be used in source() method to clone and checkout a git repo.
Docs: conan-io/docs#2419

This is a first iteration, containing only the capturing code that enables the old scm flow:

  • Very flat design, no inheritance, everything explicit.
  • Only the minimum methods to implement scm old flow
  • Powerful flexibility can implement the Git monorepo case

AFTER #10654

@memsharded memsharded added this to the 1.46 milestone Feb 16, 2022
conans/client/source.py Outdated Show resolved Hide resolved
status = self._run("status -s").strip()
return bool(status)

def get_url_commit(self, remote="origin", branch="master"):
Copy link
Contributor

@czoido czoido Feb 23, 2022

Choose a reason for hiding this comment

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

I think we should change this default to main.
Also since git>2.28 there's the init.defaultBranch configuration option that we may check using git config --get init.defaultBranch

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the name get_url_commit, it reads like get the URL of the commit and makes no sense.
About the remote and branch defaults, maybe we should make it positional (not defaulted)? I would avoid extra commands like the git config --get init.defaultBranch suggested, even more, if they depend on the git version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can avoid using the branch and just use the commit hash? Do we need the branch name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, need to find a better name.

In any case, the branch needs to be worked out. It shouldn't be necessary, and indeed is very problematic for user flows like branching, PRs, in which the branch will not be "main" or "master". So the commit should be looked for directly in the remote without a branch hint, I'll investigate if this is possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed the name to get_url_and_commit() and removed the branch argument, now checking if the commit is in any branch. Added a test with an actual branch pushed, to see it still works.

franramirez688 and others added 15 commits February 28, 2022 19:15
* first approach. Big changes in Conf

* Fixing errors

* Fixing more tests

* Deleted useless iteration

* All the tests passing

* Testing

* Added items method

* fixed rebased

* Keeping old functions for backward compatibility

* backward compatibility

* Keeping indentation

* Ordering methods and improved legacy logic

* Added fixme

* Reverted breaking changes

* Added more complete test

* Added more access Conf tests

* Added docstring

* Splitted string asserts

* Added get/pop functions

* Breaking refactor. Trying new structure

* fixing tests

* Changing tests

* Fixed tests

* Fixed Python 2.x problems

* Removed useless layer of types

* Fixed py2

* Fixed errors and added cast param to apply function to convert any type

* Added TODO

* Improved docstring

* Fixed error

* Update conans/model/conf.py

Co-authored-by: James <james@conan.io>

* Update conans/model/conf.py

Co-authored-by: James <james@conan.io>

* Added check_type and removed cast

* Changed main structure. Now simpler and more powerful

* Explicit default

* Fixed error

* Simplified compile options

* Changed all the legacy conf getitem built-ins

* Fixed test

* Fixed bad type

* Added more tests

* More tests

* Fix test for Python2

* Added more tests. Fixed corner-cases

* Added one cli test and improved boolean conf UX

* Moved str conveersion

* Added one more mechanism to other smart conversion. Added more tests

* Removed useless OR

Co-authored-by: James <james@conan.io>
* Added custom versions and descriptions for components

* Changed property to component_version. Backported to legacy PkgConfig
* WIP: Test failing

* output_folder absolute only when existing

* Fix test windows
* use absolute path

* Update conans/test/integration/environment/test_env.py

Co-authored-by: James <james@conan.io>

* fix tests

* fix test

* fix win

* fix win

Co-authored-by: James <james@conan.io>
Co-authored-by: James <james@conan.io>
@@ -491,3 +493,20 @@ def collect_libs(conanfile, folder=None):
result.append(name)
result.sort()
return result


def swap_child_folder(parent_folder, child_folder):
Copy link
Member Author

Choose a reason for hiding this comment

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

Name to be improved. Suggestions welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a generic move_folder_contents(dst_folder, folder, clean_first=True) ?

Copy link
Contributor

@czoido czoido Mar 2, 2022

Choose a reason for hiding this comment

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

I think maybe we can split the operation in two, one for removing all folders but the one we want and other for moving the folder... or move_folder_contents(dst_folder, folder, clean_first=True) is more explicit, I'm ok with that too, maybe clean_other instead of clean_first?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am annotating it to NOT document, so we can think about file methods in a more broad perspective, this PR is focused on the Git integration for sources, this is a very minor detail, just to prove it is possible to implement the mono-repo approach

@memsharded memsharded requested a review from czoido March 1, 2022 16:00
@memsharded memsharded requested a review from lasote March 1, 2022 17:03
conans/test/utils/scm.py Outdated Show resolved Hide resolved
@memsharded memsharded merged commit 3c3a718 into conan-io:develop Mar 2, 2022
@memsharded memsharded deleted the feature/new_scm_git branch March 2, 2022 19:09
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.

None yet

6 participants