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

add commit distance #19

Merged
merged 3 commits into from
Jan 9, 2024
Merged

add commit distance #19

merged 3 commits into from
Jan 9, 2024

Conversation

loriab
Copy link
Contributor

@loriab loriab commented Dec 30, 2023

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.

  • the distance info could certainly be extracted downstream from DyanamicVersion-returned info. but because of the regex match, it seems convenient to lodge it here.
  • I think the enclosing project is going to need something like the below if it doesn't itself require cmake 3.25. otherwise, the main return in DynVer doesn't actually return anything and nothing gets set for project(... VERSION ${DynVerRtn}). One can't use policy push/pop in DynVer itself b/c it exits with return().
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.25.0")
    # needed by DynamicVersion
    cmake_policy(SET CMP0140 NEW)
endif()
  • in particular, I haven't explored or even though much about how distance works with a default for the tarball case.
  • I saw you changed the cmake min to 3.20. why not 3.19 for string(JSON?

@LecrisUT
Copy link
Owner

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

cmake/DynamicVersion.cmake Outdated Show resolved Hide resolved
cmake/DynamicVersion.cmake Outdated Show resolved Hide resolved
@loriab
Copy link
Contributor Author

loriab commented Dec 30, 2023

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
Copy link
Owner

LecrisUT commented Jan 3, 2024

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

@loriab
Copy link
Contributor Author

loriab commented Jan 4, 2024

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
Copy link
Owner

LecrisUT commented Jan 4, 2024

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
Copy link
Owner

LecrisUT commented Jan 5, 2024

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:

  • 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
  • GIT_SHORT_SHA 8 character long commit short sha.

@LecrisUT LecrisUT mentioned this pull request Jan 5, 2024
4 tasks
@loriab
Copy link
Contributor Author

loriab commented Jan 5, 2024

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
Copy link
Owner

LecrisUT commented Jan 5, 2024

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
Copy link
Contributor Author

loriab commented Jan 5, 2024

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
Copy link
Owner

LecrisUT commented Jan 5, 2024

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
Copy link
Contributor Author

loriab commented Jan 5, 2024

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 LecrisUT mentioned this pull request Jan 5, 2024
@LecrisUT
Copy link
Owner

LecrisUT commented Jan 5, 2024

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
Copy link
Contributor Author

loriab commented Jan 6, 2024

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
Copy link
Owner

LecrisUT commented Jan 8, 2024

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.

cmake/DynamicVersion.cmake Outdated Show resolved Hide resolved
cmake/DynamicVersion.cmake Outdated Show resolved Hide resolved
@LecrisUT LecrisUT merged commit 6728e31 into LecrisUT:main Jan 9, 2024
11 of 15 checks passed
@loriab loriab deleted the distance branch January 9, 2024 15:36
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

2 participants