conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.24k stars 980 forks source link

[question] Inconvenient way to retrieve the scm commit with scm.Git. Why not taking the latest commit of the root folder? #15259

Closed Adnn closed 10 months ago

Adnn commented 10 months ago

We followed the Conan migration guide to use the new conan.tools.scm.Git.

In particular, this line seems to have an unfortunate consequence: git = Git(self, self.recipe_folder)

From the doc of get_commit(), this will lead to the command git rev-list HEAD -n 1 -- <self.recipe_folder>. By providing a folder to the command (instead of simply ignoring this last argument), this returns the latest commit where any file in the folder was modified. This means it is almost certainly wrong as soon as the recipe does not live at the root of the repository.

Note that we tried to use self.folders.root instead for the Git(folder), since we define it explicitly in the layout(), but this value is not available in export().

Why not returning the latest commit of the whole repository by default? Otherwise, how to get the root folder here without duplication the logic from layout()?

memsharded commented 10 months ago

Hi @Adnn

Thanks for your question

Why not returning the latest commit of the whole repository by default?

Because using repository structure similar to conan-center-index for third parties, when you have 1 repo with multiple folders, and one conanfile in each folder is very popular. Also, because it is recommended by default to have the conanfile in the root of the repo for your own packages, so the only case that this is not the optimal default is for first-party repos with conanfile not in the root.

Otherwise, how to get the root folder here without duplication the logic from layout()?

We have a test covering this use case that does this:

def export(self):
      git = Git(self, os.path.dirname(self.recipe_folder)) # Parent folder

The problem is that this executes before layout() executes. The export is layout-agnostic, layout needs the full profile and configuration information to execute, while conan export should work exactly the same for all configurations, so layout() cannot play a role here. So it is necessary to have some duplication for this use case. Moving the conanfile.py to the root folder would simplify this.

Adnn commented 10 months ago

Hi @memsharded, thank you for your great explanations.

We try to compartmentalize each "vendor" in a subfolder, but this tends to make us jump through hoops. We understand the rationale for using the recipe folder by default, but this raises two further questions:

E.g. output to substantiate the last statement:

repo/0.0.0: Exported SCM: url 'git@bitbucket.org:repo.git' at commit 'e49d13540e811f2ab0482f5f6ec3209d12de6de1' repo/0.0.0: Using git commit as the recipe revision: 8ac88d93069738e8bbc682f00540b96d8b4ce326

memsharded commented 10 months ago

What about a special value for the folder parameter (such as None), or alternatively a boolean option on get_commit(), to retrieve the latest commit of the repo as a whole (which is exactly the use case we try to address)?

Yes, I guess that is doable, easy and low risk, we can add it.

Otherwise, we see that the scm revision mode is already retrieving the latest commit of the whole repo, not only of the recipe_folder, would it be possible for client code to leverage this to get the latest whole repo commit?

There are actually 2 modes, and this was done this way to avoid breaking, otherwise we would probably have changed it. The modes are scm for the whole repo and scm_folder for the commit of the current folder. But yeah, the code inside get_commit() is really simple, just commit = self.run('rev-list HEAD -n 1 --full-history -- "."'), so using the commit from the whole repo is just dropping the latest -- "." part.

memsharded commented 10 months ago

Initial draft to check it (no guarantees yet) for Conan 2.1 in https://github.com/conan-io/conan/pull/15304

memsharded commented 10 months ago

https://github.com/conan-io/conan/pull/15304 has been merged, the repository=True argument will force to get the repo commit, not the folder commit

Adnn commented 2 months ago

@memsharded It seems there is a bug with how repository=True is handled. Testing with conan 2.5.0, we can observe that the "dirty repo" exception:

ConanException: Repo is dirty, cannot capture url and commit: C:\Users\adn\projects\Clients\AstuteGraphics\AstuteManager\astutemanager\conan

is still not thrown when a file in the repository above the recipe is touched. Which is contrary to the intent of capturing the commit for the repo as a whole (and thus, being informed when such repository as a whole is not clean).

memsharded commented 2 months ago

Hi @Adnn

I think it would be better to create a new report, with reproduction instructions, to track it better.