LecrisUT / CMakeExtraUtils

Helpful CMake modules for project management and export
GNU General Public License v3.0
7 stars 1 forks source link

add commit distance #19

Closed loriab closed 9 months ago

loriab commented 10 months ago

Here's the "distance" return I'm using in libint now applied to your reworked DynamicVersion file. It basically works, but I haven't tested it much.

LecrisUT commented 10 months ago

I saw you changed the cmake min to 3.20

It's thw minimum version in current LTS releases. Unless we get some request for backwards compatibility, I prefer to move it forward. I actually want to get to 3.24 as fast as possible because of FetchContent(FIND_PACKAGE_ARGS).

I think the enclosing project is going to need something like the below if it doesn't itself require cmake 3.25.

Goos catch. Can you add the set policy in the module? Technically we can use cmake_language(DEFER) for the pop, but setting it should be easier since it's a very banine policy that doesn't hurt downstream users (not even sure why thy made it a policy).

in particular, I haven't explored or even though much about how distance works with a default for the tarball case.

Should be fine aince the whole describe is written there. I should update the tests soon to have better coverage. I have a few more projects to wrap up and I'll look into it

loriab commented 10 months ago

I saw you changed the cmake min to 3.20

It's thw minimum version in current LTS releases. Unless we get some request for backwards compatibility, I prefer to move it forward. I actually want to get to 3.24 as fast as possible because of FetchContent(FIND_PACKAGE_ARGS).

Ok, I'll probably keep using 3.19 for Libint since it wants to stay on the non-demanding side. I agree though that the integration of find_package/FC is very nice. I got to use it on a fresh project recently, https://github.com/Einsums/Einsums/blob/main/external/rangev3/CMakeLists.txt

I think the enclosing project is going to need something like the below if it doesn't itself require cmake 3.25.

Goos catch. Can you add the set policy in the module? Technically we can use cmake_language(DEFER) for the pop, but setting it should be easier since it's a very banine policy that doesn't hurt downstream users (not even sure why thy made it a policy).

will do. looks like cmake_language(DEFER will provide a 3.19 check on the side. That'll be good b/c iirc, DynVer with <3.19 does nothing, which is confusing for new integration. :-)

in particular, I haven't explored or even though much about how distance works with a default for the tarball case.

Should be fine aince the whole describe is written there. I should update the tests soon to have better coverage. I have a few more projects to wrap up and I'll look into it

sounds good. And if there's trouble I might find it in Libint since it goes through the whole embed, export, detect cycle. Reminds me that I need to switch to the functioncall, not just the include.

LecrisUT commented 10 months ago

I am updating the tests in #21, can you rebase after that? If you need help with the tests let me know.

loriab commented 10 months ago

I think the enclosing project is going to need something like the below if it doesn't itself require cmake 3.25.

Goos catch. Can you add the set policy in the module? Technically we can use cmake_language(DEFER) for the pop, but setting it should be easier since it's a very banine policy that doesn't hurt downstream users (not even sure why thy made it a policy).

The policy push-and-defer probably isn't going to work because of https://gitlab.kitware.com/cmake/cmake/-/issues/25071 . I tried instead a block/endblock, and it didn't like that either (I could have done it wrong). Only thing that worked was the below that does change the enclosing project. Maybe you have a better strategy.

 function(dynamic_version)
+    cmake_policy(SET CMP0140 NEW)
+
     #[===[.md:
     # dynamic_version
LecrisUT commented 10 months ago

The policy push-and-defer probably isn't going to work because of https://gitlab.kitware.com/cmake/cmake/-/issues/25071 . I tried instead a block/endblock, and it didn't like that either (I could have done it wrong). Only thing that worked was the below that does change the enclosing project. Maybe you have a better strategy.

 function(dynamic_version)
+    cmake_policy(SET CMP0140 NEW)
+
     #[===[.md:
     # dynamic_version

Interesting, thanks for investigating. For CPM0140 specifically I think it's fine to set it globally. I'm not sure what usage they had in mind for making it a policy. But if you want to scope it, the function approach seems promising.

LecrisUT commented 10 months ago

One more rebase, this should fix the issues in the tests (forks generally do not have git tags). Other than that it's just the other 2 unresolved discussions. (btw, I have tabs in the documentation, that's why it is not aligned)

After this I want to make another few variable outputs:

loriab commented 10 months ago

Drat, still the testing farm is failing. I'm not seeing a clear error message.

I was hoping to get clean testing on the rebase before making further changes (incl. tabs to spaces).

PROJECT_VERSION_FULL to have a form of 2.7.4.dev42+239da2e1 ({major}.{minor}.{patch}[.dev{distance}+{short_sha}]) matching the setuptools_scm default version format

That'd be great. I always prefer .dev to .post but computing the next M.m.p always seemed iffy -- perhaps scm has conventions nowadays. On the off chance any of the cmds are helpful, here's my versioner tooling (c. 2015) https://github.com/psi4/psi4/blob/master/psi4/versioner.py . CMake is sadly behind the times in storing/expression versions. On the other hand, I've seen projects that still aren't setting their SameMajorVersion/SameMinorVersion/EXACT right even in the present M.m.p.t framework

LecrisUT commented 10 months ago

Drat, still the testing farm is failing. I'm not seeing a clear error message.

Yeah, I need to switch the logic and improve the error message, basically it fails to run DynamicVersion when importing (this) project via FetchContent (because there are no tags to describe). I thought setting Fallback would have fixed it. Can postpone this issue for later and merge it even with the failing testing-farm.

I was hoping to get clean testing on the rebase before making further changes (incl. tabs to spaces).

Check both the github runner and in testing-farm you can tick near the top to see the other tests that ran and passed. Check output.txt in the test artifact/directory, that one also shows the stdout.

That'd be great. I always prefer .dev to .post

Originally I wanted to support both, but couldn't find the documentation for how to configure setuptools-scm for the latter.

but computing the next M.m.p always seemed iffy -- perhaps scm has conventions nowadays.

Interesting, can you elaborate. I thought it was just the git describe info directly.

On the off chance any of the cmds are helpful, here's my versioner tooling (c. 2015) https://github.com/psi4/psi4/blob/master/psi4/versioner.py .

Thanks, I'll look it over a bit.

CMake is sadly behind the times in storing/expression versions. On the other hand, I've seen projects that still aren't setting their SameMajorVersion/SameMinorVersion/EXACT right even in the present M.m.p.t framework

I think I can patch upstream with the equivalent built-in support after we iron out potential issues here. A nasty one I've encountered is that the python sdist does not run git-archive so those builds just fail from pypi, but I can patch that one as well.

loriab commented 10 months ago

but computing the next M.m.p always seemed iffy -- perhaps scm has conventions nowadays.

Interesting, can you elaborate. I thought it was just the git describe info directly.

I was referring to .postN being distance beyond the most recent tag, which is what one gets from git describe. Whereas .devN is distance on the way to the next tag, so the versioner software has to make a choice whether that's M+1.0 or M.m+1 or M.m.p+1, etc. I get around this in that psi4 script by having the user commit the intended upcoming tag, but I don't pretend that that's a feature :-) scm might well have conventions on this now -- I just haven't looked into it lately. https://packaging.python.org/en/latest/specifications/version-specifiers/#summary-of-permitted-suffixes-and-relative-ordering (succeeds pep440).

LecrisUT commented 10 months ago

I was referring to .postN being distance beyond the most recent tag, which is what one gets from git describe. Whereas .devN is distance on the way to the next tag

Wow I didn't know about that. At least with yet untagged HEAD, setuptools_scm seems to put the git distance from last tag. Maybe they use a +- convention to distinguish that from the other case. I think packaging.version might also have some useful information about these conventions.

loriab commented 10 months ago

Wow I didn't know about that. At least with yet untagged HEAD, setuptools_scm seems to put the git distance from last tag. Maybe they use a +- convention to distinguish that from the other case. I think packaging.version might also have some useful information about these conventions.

Huh, hopefully that's an edge case; otherwise it looks like they're flouting their own advice. Requiring projects that use your DynVer to have at least one qualifying tag seems reasonable to me, as it's something the consuming developers can do.

If the scm conventions aren't explicit somewhere, I wonder if it'd be better to stick with postN.

LecrisUT commented 10 months ago

I've opened the issue to investigate the version generation. Let's discuss it more after this when it's time to implement that :)

loriab commented 10 months ago

If you're ok with the failing testing-farm, this is good to merge from my view. I decided not to do the tab->space here because it contaminates the diff and might conflict with the other PR. I do think it'd be good to do sometime, at least on the code (rather than docs) lines.

Any other requested changes I overlooked (or to add)?

LecrisUT commented 9 months ago

If you're ok with the failing testing-farm

Yeah, that's fine with me

I decided not to do the tab->space here because it contaminates the diff and might conflict with the other PR. I do think it'd be good to do sometime, at least on the code (rather than docs) lines.

Yeah, I should have edited all of them, I'll do it after this one. But, as long as it's a single commit it's fine anywhere. Also don't worry about merge conflicts, my IDE can easily handle these.

Any other requested changes I overlooked (or to add)?

Do you want to try your hand at writing/editing the tests? Otherwise, I will handle it in a next PR.