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

Added .git/index to the CMAKE_CONFIGURE_DEPENDS #2481

Merged
merged 1 commit into from
Feb 9, 2020

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Feb 2, 2020

… to update the version if the git branch or revision changes.

Fixes https://bugs.launchpad.net/mixxx/+bug/1861548

@Holzhaus
Copy link
Member

Holzhaus commented Feb 3, 2020

I'm not sure that .git/index is the correct file. It also includes the staging area, so that changes to staged file would also trigger a version change:

$ sha256sum .git/index
2eca1e2be4ab51c3fe1a1e3fb96fd1261b1796dbec8caaf29a7e73ba2362fbd7  .git/index
$ touch foo
$ git add foo
$ sha256sum .git/index
db2c251b4e78a8d5a4d1629ee5a011c7ba5a933ccdc7ddf3c9cccd681de4dd91  .git/index

Do we want that behaviour?

If it's just about branches and commits, I think we need to read .git/HEAD and then use both .git/HEAD and the referenced file:

# When on a branch
$ cat .git/HEAD
ref: refs/heads/dont-stop-track-on-unconfigured-passthru
$ cat .git/refs/heads/dont-stop-track-on-unconfigured-passthru
bed4a876d8f37ce11896b3b610f1a0acbe2ff0b9

# In "detached HEAD" mode
$ cat .git/HEAD
67f7cc05d564647d6534ef21ec60e5fb6fc90211

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2020

./git/index is just the easy solution that works.
Reading one file and check on other is too much effort for just to save a second build time after messing with the index for me.

However this is actually not a full solution, because modified files are not considered to create the + prefix. Do you have an idea how to fix that?

@Holzhaus
Copy link
Member

Holzhaus commented Feb 4, 2020

The problem is that the git commands are run at configure time, not build time. I could rewrite it a bit. By the way, is there a reason we don't use git describe --always --dirty?

@daschuer
Copy link
Member Author

daschuer commented Feb 4, 2020

No, except that changing this will disturb our version counting.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 4, 2020

By the way, you can work around it by using:

$ cmake .. && cmake --build .

The reconfiguration should be quick because most of the variables are cached.

@daschuer
Copy link
Member Author

daschuer commented Feb 5, 2020

Yes, right. That is also an argument to just merge this. Because compared to configure every time, it does it only if the git index changes.
.. anoying wrong revision bug solved and we can proceed.

@daschuer
Copy link
Member Author

daschuer commented Feb 9, 2020

@Holzhaus what do you think? Merge?

@Holzhaus
Copy link
Member

Holzhaus commented Feb 9, 2020

I'm sure there a better way by adding a custom command target, but as a temporary workaround we can merge this.

@daschuer
Copy link
Member Author

daschuer commented Feb 9, 2020

Ok, thanks. Than please merge. :-)

@Holzhaus Holzhaus merged commit 798bb1f into mixxxdj:master Feb 9, 2020
@daschuer daschuer deleted the cmake_git branch September 26, 2021 17:37
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.

2 participants