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

Fix: git-hash injection on every build #1641

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Conversation

buhtz
Copy link
Member

@buhtz buhtz commented Feb 13, 2024

Dear @ptilopteri,
does this solve the problem?

Close #1640
Close #1637

@buhtz buhtz requested a review from aryoda February 13, 2024 16:21
@ptilopteri
Copy link

ptilopteri commented Feb 13, 2024 via email

@bit-team bit-team deleted a comment from ptilopteri Feb 13, 2024
@buhtz
Copy link
Member Author

buhtz commented Feb 13, 2024

Dear ptilopteri,

please try to use correct upper case letters. Is is more gentle to the eyes of your readers.

didn't work or I don't know how to apply?

I never used the email interface of Microsoft GitHub or the patch/diff files it provides. So I can not be of assistance here.
I do work directly with "git". Just clone my forked repository, checkout the bugfix branch and got for it.

Try it with that:

$ git clone https://github.com/buhtz/backintime.git
$ cd backintime
$ git checkout fix/fix1637
$ cd common && ./configure && make && sudo make install
$ cd ../qt && ./configure && make && sudo make install
$ backintime --version

@buhtz buhtz self-assigned this Feb 13, 2024
@buhtz buhtz added this to the Upcoming release (1.5.0) milestone Feb 13, 2024
@ptilopteri
Copy link

ptilopteri commented Feb 13, 2024 via email

@buhtz
Copy link
Member Author

buhtz commented Feb 13, 2024

does that still need to be patched with ~/1592_workaround.txt ?

I am not aware of a file named like that. If you refer to Issue #1592 (assigned to aryoda, not me) I would answer no, because that Issue is not closed (solved) yet.

@aryoda
Copy link
Contributor

aryoda commented Feb 13, 2024

does that still need to be patched with ~/1592_workaround.txt ?

I am not aware of a file named like that. If you refer to Issue #1592 (assigned to aryoda, not me) I would answer no, because that Issue is not closed (solved) yet.

@ptilopteri The patch is only a workaround and must still be applied for each BiT version built from the dev branch until the issue is closed (and this requires more time, see #1592 (comment)). T

@aryoda
Copy link
Contributor

aryoda commented Feb 13, 2024

@buhtz The current code of this PR does call the updateversion.sh in the build target (not in the install target) of make.

I think both targets have pros and cons and we simply have to decide (or toss a coin ;-)

build target:

  • Pro: Git hash already injected without installation (so shown when BiT is started directly from the repo)
  • Con: Later commits or switching the branch after make do not update the version number (= wrong Git hash is shown!)
  • Con: BiT is not meant to be used for other purposes than development when started from the Git repo (eg. on a clean system without an installed "old" BiT the servicehelper.py is not running and cron scheduling is not possible)

install target:

  • Pro: Ensures that the Git hash is correct (representing the HEAD of the Git branch)
  • Con: No Git hash in the version number if started directly from Git repo

I am "voting" for the install target since it feels more secure...

common/configure Outdated
@@ -161,7 +161,8 @@ printf "DEST=\$(DESTDIR)\$(PREFIX)\n\n" >> ${MAKEFILE}

printf "all:\tbuild\n\n" >> ${MAKEFILE}

printf "build:\ttranslate compress\n\n" >> ${MAKEFILE}
printf "build:\ttranslate compress\n" >> ${MAKEFILE}
printf "\t(cd .. && ./updateversion.sh)\n\n" >> ${MAKEFILE}
Copy link
Contributor

@aryoda aryoda Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changing the directory within the script may have side effects on all other relative paths in commands called subsequently (= using the wrong path) - if any are called

AFAIR a solution for this would be to cd in the called updatevesion.sh script (into the folder of the script).
I need to search for a bash code for this (can't find it ATM).

Or maybe: Make sure this line is always called at the end (and a comment should explain this and warn not to move it into the middle of the generated Makefile code). Currently it looks like it is the last command of the target...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is IMHO no problem because this "cd" is temporary because of the (...). It is activte only inside the brackets.

$ ls && (cd .. && ls) && ls
huhu.txt
downdir/  infoobar.txt
huhu.txt

It seems to be POSIX and works on several shells. The make system do use "sh" by default. There is also the pushd command but it works only on bash. You would do ls && pushd .. && ls && pushd +1 && ls on bash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic - looks very good (didn't know the parentesis semantics until now :-)

@buhtz
Copy link
Member Author

buhtz commented Feb 14, 2024

Thanks for reviewing.

I moved it from "build" to "install" target. But I kept it as the first action in that target because it need to be done before the py-files are copied via install command.

The "cd" is IMHO no problem because is temporary in that script.

@buhtz buhtz merged commit 75089bd into bit-team:dev Feb 15, 2024
1 check passed
@buhtz buhtz deleted the fix/fix1637 branch February 15, 2024 07:40
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.

Identify BiT dev build by git hash
3 participants