Dirty flag fix #3822
Dirty flag fix #3822
Conversation
Why? What is the purpose? If I apply a custom patch without committing, and then give the patch + source code to someone else, ideally the build would still be producible. |
CMakeLists.txt
Outdated
| @@ -321,7 +321,7 @@ endif() | |||
|
|
|||
| # Get the current commit ref | |||
| execute_process( | |||
| COMMAND git describe --tags --always --dirty=-modified | |||
| COMMAND git describe --tags --always | |||
There was a problem hiding this comment.
So how it is possible to distinguish a patched Mixxx from an unpatched one?
There was a problem hiding this comment.
I have removed it here, because it is unreliable at this point. The dirty flag is only calculated at configure time.
If you build a new commit, it is calculated as unmodified. When a source file has been changed, it is not reconfigured.
I have fixed it by moving the dirty check to cmake/builddate.cmake and let it depend on mixxx-lib. This works reliable except for main.cpp which is OK I think.
There was a problem hiding this comment.
We could make sure to always run the git info header configuration. We can use https://cmake.org/cmake/help/latest/command/add_custom_target.html for this. What do you think?
There was a problem hiding this comment.
Idea: We always run the command during every build but only configure a temp file, then compare the content of the present configured file with the temp file. If they don't match, overwrite the configured file with the temp file. That way there should be no unnecessary recompilations and the dirty-flag would work for free, without the complicated hack for detecting the dirty state.
I can take care of that If you want (you're probably busy with investigating the PPA). Just let me know.
As I said I'd also like to split the version info into two parts:
- Stuff that often changes (i.e. git describe, date) => here we can use the approach I listed above.
- Stuff that rarely changes (CMAKE_PROJECT_VERSION) => This should be split off into a separate header file that can be included in
defs_urls.hfor example, so that we can replace the hardcoded 2.3 version there on the preprocessor level.
Do you agree?
There was a problem hiding this comment.
What would be improved?
I am afraid it will not work well and is more hacky than the current solution which uses the normal dependency tree of the underlying build system.
Idea: We always run the command during every build but only configure a temp file.
I like to have a solution that only kicks in when we create a new binary anyway.
That way there should be no unnecessary recompilations and the dirty-flag would work for free.
Not really, I guess the current solution is much faster since It already has no unnecessary recompile.
Splitting up the versions into two files makes only sense if they go into different object files.
This is the case currently. The often changing dirty flag and date are moved into the builddate.cpp.o and the rest into versiondtore.cpp.o.
I don't think that writing the version will be give a notable delay. So we may move everything from version.h into a include free version.cpp.o and put this into an own lib replacing builddate.cpp bx following the same pattern implemented here.
What do you think?
There was a problem hiding this comment.
What would be improved?
git describe --dirty would work.
I am afraid it will not work well and is more hacky than the current solution which uses the normal dependency tree of the underlying build system.
It would use the dependency tree of the build system.
If the values of git describe or the date changed, then we overwrite the configured header file -> Therefore the file time changes and the build system will recompile all files that include it.
It's not really hacky IMHO.
Not really, I guess the current solution is much faster since It already has no unnecessary recompile.
If no source file changes (i.e. if git describe and date did not change), then the file time will stay the same and the build system shouldn't recompile/relink the mixxx binary.
Splitting up the versions into two files makes only sense if they go into different object files.
That is the point. I'd like to use CMAKE_PROJECT_VERSION_MAJOR and CMAKE_PROJECT_VERSION_MINOR in defs_urls.h. Right now this is not possible without recompiling every compilation unit that includes defs_urls.h on every single commit.
There was a problem hiding this comment.
But i don't really care, as long as you re-add the --dirty=-modified flag. Right now it's not possible to see if the user built a version with local modifications.
There was a problem hiding this comment.
The "-modified" appendix is already in the about box with this PR and that reliable. That was the reason why I have stared this PR.
The root issue we are facing here, is that we use git describe as a package name and CPack is configured at configure time.
The system is reconfigured whenever you commit. The first build after commit will produces a new "git describe" string and is almost always not dirty. All following changes to the source do not reconfigure the project. That's why the dirty flag from configure time is unreliable and I have --dirty=-modified in the CMakeList.txt.
For a reliable dirty flag we need to check it whenever a new binary is created, This is done here with an independent git call. The result is appended to the git describe string just like git would do it.
We have now the issue, that the package name does not have the modified appendix with this branch. I consider this not and issue, because it is an edge case and a solution requires a hack to read the modified flag from a file during CPack time, I do avoid to maintain. Is that OK?
It would use the dependency tree of the build system.
The penalty would be that the cmake script running git described is executed on every build. The project is reconfigured, whenever the dirty flag changes, not only if the commit hash changes. It is also reconfigured and forces an unnecessary rebuild if a file is changed that would normally not force a rebuild, like skins and controller mappings.
That the reason why I am not to fond of it.
defs_urls.h
We could remove all version strings form defs_urls.h and compose the Urls on the fly using version store. Would it work?
We may also put getters to VersionStrore and include defs_urls only from there.
Or do you need the defs_urls for something else?
| @@ -0,0 +1,7 @@ | |||
| #pragma once | |||
|
|
|||
| class BuildDate { | |||
There was a problem hiding this comment.
Why is this an extra class? Maybe we should make a gitinfo.h.in instead, so that we split off data that changes each commit (git describe, commit date, etc.) from data that is fixed most of the time (CMAKE_PROJECT_VERSION).
That would allow to include the regular version.h in defs_urls.h so that we can use CMAKE_PROJECT_VERSION_MAJOR and CMAKE_PROJECT_VERSION_MINOR in the manual URL.
The idea was to be able to distinguish different patched versions, that are created during debug builds without commuting.
Yes this case in not covered. My Idea was to use git for reproducible builds. A tarball build is not reproducible this way. Maybe we need to move to a file time based approach to create a reproducible date for this. |
This is done for performance reason. The object builddate.cpp.o is now as tiny as possible and QT free. It compiles in no time compared to a cpp file with QT includes. This file is build in addition to any other a single file touched so it should kept small.
I cant follow here, but i would be o deal to move more defines into this object, as long we don't need to include any system header. |
…tate Allow to read date form version.h
|
I have started over. Now it works like this
Does this work for you? |
|
The question is: What do we want to archieve? We use the git describe version to make bug reports more useful, so that we can tell users "give us this string and we know what version is used".
Modified branches are an edge case for developers, and we could easily keep using the git commit date (even though there are uncommitted changes). For tarballs I think I doesn't hurt to leave it on "unknown". Can you explain the purpose of using the build date? I'm asking because I don't think it's actually helpful, we won't be able to use bug reports from people that use the tarball anyway right now, because we don't know what version they used. I think it would be more helpful if we implemented a way to override the git version and the git commit date from the command line (e.g. |
Since we have the commit count and a developer has git under his finger tips, the additional commit date is a almost useless info for a developer. I do not expect anyone opening the Mixxx about box and look for the commit date.
Right, it just looks a bit like weakness and the current date is a workaround. I think it would be more helpful if we implemented a way to override the git version and the git commit date from the command line. For the PPA build I have patched the version.h.in during the source export. This way the git version is shown in the patch file.
I will add it here. This way the developer has the full power to decide for reproducible builds or a time stamp fall back. |
|
Does it work for you? |
|
Yes. |
|
Done |
|
Summary: I think the discussion melts down to the requirements vs. complexity and build time This solution features the following discussed points:
Can you post an edited list if your requirements are different? I am sure we find than a good solution without too much hacks, that also covers the URL issue. I have actually no strong opinion here if the penalty on null builds is negligible. Linking mixxx-lib is not negligible. The proposed extra "git describe --dirty" on every build would be OK for good reasons. Please note that the "--dirty" flag makes the call slow especially on windows. I can even live with not considering the dirty flag at all. Since the move to cmake it is broken and no one has noticed it. |
|
I think the dirty flag is more important than using git describe in the package filename. For our deployment the package name is irrelevant anyway. And ideally it would consider installed resources because we might get bug reports concerning skins where the version is important. I'll make another PR, so that we can compare both implementations, ok? |
Yes that would be OK. Maybe you can outline some details before that we can straight away use your version?
I am not opposing, but to be clear: the user can change the files at any time. So we need well consider if the extra build time on the contributor machine is well spend for to add this unreliable info. For my understanding you propose:
is that correct? |
Yes.
You mean build date? I think it does prevent reproducible builds when building from a tar.gz, so I don't think we can keep it in the long term. For now, I think we can add it because our builds are not reproducible anyway.
I don't really care about this, because we rename the packages before deployment anyway, and distro maintainers will probably use their own packaging scripts or rename the package afterwards. Most devs won't build packages anyway because it's much easier to just run mixxx from the build dir. So I'd even be okay with just using the CMAKE_PROJECT_VERSION or something (although git describe is obviously better).
I had the impression that only numeric values were allowed, so I don't think this is possible.
Of course I'd like to avoid that penalty as much as possible. I have different solutions in mind, and the question is what you consider a "null build". If no source file was changed and no commit was made, mixxx should not reconfigure/relink, if that's what you mean.
I'd like to make sure that the dirty flag is correct. There are various solutions to this, that may or may not reconfigure (the latter is only possible if we don't change CPACK configuration on dirty flag changes, just update the about window variable - I'd be okay with that, too). Considering that reconfiguration is pretty cheap (less than a second on my machine - is this slow for you?), I don't really care. Personally, I always build with "CTRL-R cmake " (=> |
Currently it is not reproducible by default, but if you set GIT_BUILD_DATE and GIT_DESCRIBE like the package maintainers should do, it is reproducible. The benefit is that we can distinguish different dirty builds. I like this solution, but I am not insist.
There is a "Patched" flag we may use.
Yes exactly. The speed of reconfigure depends on what is done. A null-reconfigure is here < 1s which is not an issue. I am just concerned of the extra time during daily work. For good reasons we can effort some time.
That is implemented here :-) Did you consider the installed resources and the dirty flag? |
|
Please have a look at #3832 and let me know if that fits your requirements. I didn't add build time info, that can be a follow-up PR if you want. |
|
Does "res" work recursive into the res tree? Using the sources directly as dependency bypasses the make dependency tree. So I guess it will not be reconfigured, a header file changes. I think you first idea is more reasonable to write a dummy file that changes when the git describe string changes. And use that as configure dependency. This can be a cmake script that has an outputs that never exists an this way the target is executed on every single build. |
|
I think configure file does the file time hack internally already. For my feeling we have now the components on the table. Do you have interest to put them together in a new branch or should I integrate them here? |
Since the git folder is not changed when a source file changes without a commit, cmake was not reconfigured and the dirty flag was not updated.
This PR uses an extra cmake script that is executed whenever mixxx-lib has been updated, to update the dirty state.
In case of dirty. it used the current time instead of the commit time as date in the about box.