git_helpers: add fallback for inconsistent describe #2

proppy · 2022-12-22T06:47:36Z

Fixes #1

Before:

>>> import conda_build_prepare.git_helpers
>>> conda_build_prepare.git_helpers.get_latest_describe_tag('.')
'Public_Release-7.6.0-77-g50daf8e8e069363aea2fe91cc8012a8d3038bd87'

After:

>>> import conda_build_prepare.git_helpers
>>> conda_build_prepare.git_helpers.get_latest_describe_tag('.')
'Release-7.6.0'

proppy · 2022-12-22T18:01:12Z

@mithru @ajelinski can you take a look?

mithro

I'm not sure this is right but I'm not sure if it is wrong either :-P

mithro

I'm going to assume this works?

Would be nice to have tests but I don't think we want to block things to add tests.

conda_build_prepare/git_helpers.py

proppy · 2022-12-23T12:40:32Z

Would be nice to have tests but I don't think we want to block things to add tests.

Added tests and simplified implementation.

proppy · 2022-12-23T12:45:58Z

@ajelinski @mithro PTAL, added some tests

mithro

Some minor comments on the testing.

mithro · 2022-12-25T00:16:18Z

test/test_git_helpers.py

+    gh._call_custom_git_cmd(filepath.parent, f'commit -m {msg}')
+    if tag:
+        gh._call_custom_git_cmd(filepath.parent, f'tag {tag}')
+    time.sleep(COMMIT_DELAY)


You might need to explain why you need time.sleep and COMMIT_DELAY?

otherwise git get confused about the sorting with all the commit dates being the same (it doesn't got below second resolution).

I'll add a comment.

mithro · 2022-12-25T00:16:49Z

test/test_git_helpers.py

+
+
+def git_commit_and_tag(filepath, msg, tag=None):
+    now = time.time()


Why not use tempfile?

I do use tmp_path fixture from pytest in the test function, see https://docs.pytest.org/en/7.1.x/how-to/tmp_path.html for more details.

The date here is used for the file content (not the name).

mithro · 2022-12-25T00:17:35Z

test/test_git_helpers.py

+    time.sleep(COMMIT_DELAY)
+
+
+def test_get_first_reachable_tag(tmp_path):


I think you probably want to put the setup into a def setup_test_repo type function?

you mean the git init + git config? Maybe we can do this when there is actually more than one test that share the same setup.

Yeah, mostly because it wasn't clear to see what was the actual test verse what was just needed to get things set up for the test.

mithro · 2022-12-29T17:02:22Z

@proppy - What do we need to do to get this merged?

proppy · 2022-12-29T17:03:15Z

@proppy - What do we need to do to get this merged?

We need to address the remaining comments :)

ajelinski · 2023-01-15T11:55:22Z

conda_build_prepare/git_helpers.py

    try:
-        return _call_custom_git_cmd(git_repo, 'describe --tags --abbrev=0', quiet=True)
+        # list reachable tag sorted by commit date
+        tags = _call_custom_git_cmd(git_repo, 'tag --merged').splitlines()


Perhaps it depends on the git configuration but for me this is sorted lexicographically.
For example for hidapi (https://github.com/libusb/hidapi) this is definitely not a valid replacement:

$ git describe --tags --abbrev=0 hidapi-0.13.1 $ git tag --merged hidapi-0.1.0 hidapi-0.10.0 hidapi-0.10.1 hidapi-0.11.0 hidapi-0.11.1 hidapi-0.11.2 hidapi-0.12.0 hidapi-0.12.0-rc1 hidapi-0.12.0-rc2 hidapi-0.13.0 hidapi-0.13.1 hidapi-0.2.2 hidapi-0.2.3 hidapi-0.2.4 hidapi-0.3.0 hidapi-0.5.0 hidapi-0.5.1 hidapi-0.5.2 hidapi-0.6.0 hidapi-0.7.0 hidapi-0.8.0-rc1 hidapi-0.9.0 hidapi-0.9.0-rc1

I tried using different --sort options but they don't seem very stable to me (shouldn't creatordate be the same as taggerdate or committerdate?):

$ git tag --sort=taggerdate --merged hidapi-0.1.0 hidapi-0.10.0 hidapi-0.10.1 hidapi-0.11.0 hidapi-0.11.1 hidapi-0.11.2 hidapi-0.12.0 hidapi-0.12.0-rc1 hidapi-0.12.0-rc2 hidapi-0.2.2 hidapi-0.2.3 hidapi-0.2.4 hidapi-0.3.0 hidapi-0.5.0 hidapi-0.8.0-rc1 hidapi-0.9.0 hidapi-0.9.0-rc1 hidapi-0.5.1 hidapi-0.5.2 hidapi-0.6.0 hidapi-0.7.0 hidapi-0.13.0 hidapi-0.13.1 $ git tag --sort=committerdate --merged hidapi-0.13.0 hidapi-0.13.1 hidapi-0.5.1 hidapi-0.5.2 hidapi-0.6.0 hidapi-0.7.0 hidapi-0.1.0 hidapi-0.2.2 hidapi-0.2.3 hidapi-0.2.4 hidapi-0.3.0 hidapi-0.5.0 hidapi-0.8.0-rc1 hidapi-0.9.0-rc1 hidapi-0.9.0 hidapi-0.10.0 hidapi-0.10.1 hidapi-0.11.0 hidapi-0.11.1 hidapi-0.11.2 hidapi-0.12.0-rc1 hidapi-0.12.0-rc2 hidapi-0.12.0 $ git tag --sort=creatordate --merged hidapi-0.1.0 hidapi-0.2.2 hidapi-0.2.3 hidapi-0.2.4 hidapi-0.3.0 hidapi-0.5.0 hidapi-0.5.1 hidapi-0.5.2 hidapi-0.6.0 hidapi-0.7.0 hidapi-0.8.0-rc1 hidapi-0.9.0-rc1 hidapi-0.9.0 hidapi-0.10.0 hidapi-0.10.1 hidapi-0.11.0 hidapi-0.11.1 hidapi-0.11.2 hidapi-0.12.0-rc1 hidapi-0.12.0-rc2 hidapi-0.12.0 hidapi-0.13.0 hidapi-0.13.1

The --sort=creatordate option seems to be what we look for but I'd need to test this on all our packages if it's really correct. I have a feeling the date of creation of either tag or commit won't necessarily point to the tag "closest" to the current HEAD.

Therefore I started thinking about other options.
Since for Xyce the Public_Release-7.6.0 name returned by git describe --tags is invalid I didn't find any way to get to the actual tag name from it.

Maybe we should simply call git describe --tags and grep the warning message to return the "is really '(.*)' here" name:

warning: tag 'Public_Release-7.6.0' is really 'Release-7.6.0' here

I don't like this option very much but at least we don't need to worry about tag sorting which might be tricky.
WDYT?

ajelinski · 2023-01-16T17:54:03Z

FYI: I've just pushed a fix for the issue causing the CI to fail.

This is my proposal how to fix the #1 issue as I'd really prefer to still use git describe: b7ef41f
Unfortunately I'll be OOO for a week so feel free to include this commit to your PR and merge if you agree on the solution.

ajelinski · 2023-01-16T17:56:35Z

test/test_git_helpers.py

+
+    gh._call_custom_git_cmd(repo, 'checkout main')
+    git_commit_and_tag(repo / 'foo', 'third', tag='v0.2')
+


To make sure this isn't just sorted lexicographically perhaps let's also add a v0.10 tag?

Otherwise the test file LGTM.

git_helpers: add fallback for inconsistent describe

02dd022

proppy mentioned this pull request Dec 22, 2022

sim: add xyce hdl/conda-eda#263

Open

mithro reviewed Dec 22, 2022

View reviewed changes

conda_build_prepare/git_helpers: use git tag instead of describe

917639f

mithro approved these changes Dec 23, 2022

View reviewed changes

conda_build_prepare/git_helpers.py Outdated Show resolved Hide resolved

conda_build_prepare/git_helpers.py Show resolved Hide resolved

test: add git_helpers unit tests

17e71e4

proppy added 2 commits December 23, 2022 21:41

test/test_git_helpers: remove unused imports

06ea4bd

test/test_git_helpers: add local git config

41ecf06

mithro approved these changes Dec 25, 2022

View reviewed changes

ajelinski reviewed Jan 15, 2023

View reviewed changes

ajelinski reviewed Jan 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git_helpers: add fallback for inconsistent describe #2

git_helpers: add fallback for inconsistent describe #2

proppy commented Dec 22, 2022 •

edited

Loading

proppy commented Dec 22, 2022

mithro left a comment

mithro left a comment

proppy commented Dec 23, 2022

proppy commented Dec 23, 2022

mithro left a comment

mithro Dec 25, 2022

proppy Dec 25, 2022

mithro Dec 25, 2022

proppy Dec 25, 2022

mithro Dec 25, 2022

proppy Dec 25, 2022

mithro Dec 25, 2022

mithro commented Dec 29, 2022

proppy commented Dec 29, 2022 •

edited

Loading

ajelinski Jan 15, 2023

ajelinski commented Jan 16, 2023

ajelinski Jan 16, 2023

ajelinski Jan 16, 2023



		def git_commit_and_tag(filepath, msg, tag=None):
		now = time.time()

		time.sleep(COMMIT_DELAY)


		def test_get_first_reachable_tag(tmp_path):


		gh._call_custom_git_cmd(repo, 'checkout main')
		git_commit_and_tag(repo / 'foo', 'third', tag='v0.2')

git_helpers: add fallback for inconsistent describe #2

Are you sure you want to change the base?

git_helpers: add fallback for inconsistent describe #2

Conversation

proppy commented Dec 22, 2022 • edited Loading

proppy commented Dec 22, 2022

mithro left a comment

Choose a reason for hiding this comment

mithro left a comment

Choose a reason for hiding this comment

proppy commented Dec 23, 2022

proppy commented Dec 23, 2022

mithro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mithro commented Dec 29, 2022

proppy commented Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

ajelinski commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy commented Dec 22, 2022 •

edited

Loading

proppy commented Dec 29, 2022 •

edited

Loading