Ticket #47967

gitrev is wrong in freeciv-server -v

Open Date: 2023-04-30 06:58 Last Update: 2024-02-09 15:33

Reporter:
Owner:
(None)
Type:
Status:
Open
Component:
MileStone:
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
None
File:
4

Details

S3_1

The value return buy freeciv-server -v is an old reference (in example 164 commit behind origin/S3_1 pulled today)

$ LANG=en ./server/freeciv-server -v
Encodings: Data=UTF-8, Local=ISO-8859-1, Internal=UTF-8
Freeciv version 3.1.0-beta1+ (beta version) (modified 75200a454) 

$ git log origin/S3_1...75200a454 | grep commit | wc -l
164

It seems that the values of bootstrap/generate_gitrev.sh -> ./common/fc_gitrev_gen.h are not taken into account and we have a fallback from somewhere.

Ticket History (3/24 Histories)

2023-04-30 06:58 Updated by: alain_bkr
  • New Ticket "gitrev is wrong in freeciv-server -v" created
2023-04-30 07:07 Updated by: cazfi
Comment

Can you check:
- If there are multiple fc_gitrev_gen.h files (e.g. one in source directory, one in build directory)
- Timestamp of common/version.o (i.e. has it been rebuilt after you did 'git pull')
- Timestamp of fc_gitrev_gen.h (i.e. has version.o been rebuilt *after* that header has been regenerated)

2023-04-30 16:52 Updated by: None
Comment

I build in a separate directory

 git rootdir : /Big/Games/freeciv_S3_1/
 builddir    : /Big/Games/freeciv_S3_1/build_clang-15
 
make distclean
../autogen.sh --enable-gitrev --enable-debug=yes ....

So as you expected there are have 2 fc_gitrev_gen.h files ( one in source directory ../ , one in build directory ./ )

OK i got it : I remove the one in source directory , probably a sequel of a previous build in root dir, and now i have what i want.

The generated file ./common/fc_gitrev_gen.h is not erased by make distclean, i guess this explains my trouble

2023-04-30 23:45 Updated by: None
Comment

make maintainer-clean does erase the generated fc_gitrev_gen.h

btw, i have improved bootstrap/generate_gitrev.sh , to find the most recent origin/S3_1 commit , and head commit , and count the number of local commit (here 6), and add "+ modified" if there are uncommited local changes

$ ./fcser -v
Encodings: Data=UTF-8, Local=UTF-8, Internal=UTF-8
Freeciv version 3.1.0-beta1+ (version bêta) (origin/S3_1 373049274 HEAD 7ef4e956f (+6) + modified ) 
2023-05-01 01:57 Updated by: cazfi
Comment

Reply To (Anonymous)

add "+ modified" if there are uncommited local changes

- There's a regression that "+ modified" does not get translated (in general: is there some reason how you split the information between REV1 and REV2?)
- Need to update comments in fc_gitrev_gen.h.tmpl to match (and I notice that translatability of "modified " was something already missing from there)
- I have not yet checked if such a long string makes sense for all freeciv_name_version() and fc_git_revision() callers, but I suspect some adjustments are needed for latter at least

2023-05-01 02:12 Updated by: alain_bkr
Comment
 $ ./fcser -v
 Encodings: Data=UTF-8, Local=UTF-8, Internal=UTF-8
 Freeciv version 3.1.0-beta1+ (version bêta) (origin/S3_1 373049274 HEAD 7ef4e956f (+6) + modified ) 

It works like a charm on my machine. :-)

(Edited, 2023-05-01 02:13 Updated by: alain_bkr)
2023-05-01 03:52 Updated by: alain_bkr
Comment

Reply To cazfi

Reply To (Anonymous)

add "+ modified" if there are uncommited local changes

- There's a regression that "+ modified" does not get translated

OK i 'll remove the + and use translate

(in general: is there some reason how you split the information between REV1 and REV2?)

yes,

  • it was split REV1/REV2
  • The REV2 gives the same hash as before the patch.

- Need to update comments in fc_gitrev_gen.h.tmpl to match (and I notice that translatability of "modified " was something already missing from there)

done, i attach the corrresponding patch

- I have not yet checked if such a long string makes sense for all freeciv_name_version() and fc_git_revision() callers, but I suspect some adjustments are needed for latter at least

It just works for the displayed version in GUI or command line, these are strings anyways, just longer ones.

2023-05-01 04:09 Updated by: cazfi
Comment

Reply To alain_bkr

- I have not yet checked if such a long string makes sense for all freeciv_name_version() and fc_git_revision() callers, but I suspect some adjustments are needed for latter at least

It just works for the displayed version in GUI or command line, these are strings anyways, just longer ones.

Yeah, looking for the callers, the only one not used "just for display" is the lua API one. If someone has custom script parsing the version number assuming specific format, they have taken the risk themselves.

Existing string has filled entire horizontal space on sdl2-client on lower resolution displays, but likely that's something we can resolve separately.

2023-05-01 23:24 Updated by: alain_bkr
Comment

1/ i don't know how to translate the word "modified" inside the string, and i would advocate it is not that important, because it occurs only when one has some uncommitted changes, so i guess that person would understand :-)

2/ maybe add common/fc_gitrev_gen.h to make clean list ? (i don't know how)

(Edited, 2023-05-01 23:27 Updated by: alain_bkr)
2023-05-02 01:31 Updated by: cazfi
Comment

Reply To alain_bkr

1/ i don't know how to translate the word "modified" inside the string, and i would advocate it is not that important, because it occurs only when one has some uncommitted changes, so i guess that person would understand :-)

Existing code translates REV1:

  fc_snprintf(buf, sizeof(buf), "%s%s",
              translate ? _(FC_GITREV1) : FC_GITREV1, FC_GITREV2);

The string is collected from translations/Strings.txt

Not that it would be important, but it shouldn't be that hard either.

2023-05-02 09:01 Updated by: cazfi
  • Milestone Update from (None) to 3.0.8 (closed)
  • Component Update from General to Bootstrap
  • Type Update from Bugs to Patches
  • Priority Update from 7 to 5 - Medium
Comment

Reply To alain_bkr

2/ maybe add common/fc_gitrev_gen.h to make clean list ? (i don't know how)

Thought this was the bug this ticket originally was about, there's now so much more discussion about the improvement patch, that I did ticket splitting so that #47976 is about the bug.

2023-05-02 17:18 Updated by: None
Comment

Reply To cazfi

Reply To alain_bkr

1/ i don't know how to translate the word "modified" inside the string, and i would advocate it is not that important, because it occurs only when one has some uncommitted changes, so i guess that person would understand :-)

Existing code translates REV1:
{{{ fc_snprintf(buf, sizeof(buf), "%s%s", translate ? _(FC_GITREV1) : FC_GITREV1, FC_GITREV2); }}} The string is collected from translations/Strings.txt Not that it would be important, but it shouldn't be that hard either.

I don't know how to implement this, and i think it is unimportant, and a waste of time and energy (as i explained, people seeing this are developers with local uncommitted changes, so they will understand "modified").

For me my 2 patches are ready for production :-).

If you believe it should be implemented, please show me how to translate a substring.

2023-05-25 14:41 Updated by: cazfi
  • Owner Update from (None) to cazfi
2023-05-27 02:59 Updated by: cazfi
Comment

One should use 'git diff' to check that the changes they are about to 'git add' are what they expect (no debug code forgotten in, etc.) At least in any environment where I work, 'git diff' also shows trailing spaces as colored blocks (screenshot attached, showing couple of trailing spaces in these patches)

2023-05-27 03:26 Updated by: cazfi
Comment

Working on this, but it's taking a bit head scratching to figure out what you mean to happen, e.g., with local branches for which there's no similarly named branch on a remote named 'origin'

2023-05-27 04:16 Updated by: cazfi
Comment

Fixed some more minor issues. Not a commit candidate.

---

- Removed trailing spaces
- Removed "common/" in a comment talking about "common/fc_gitrev_gen.h" - that's not the path e.g. in meson based builds
- "optionnally" -> "optionally"
- Removed tabs
- Separated third part, GITREV3, and made handling of "modified" via that one
- While touching this script, also removing deprecated x-prefixing ( if test "x$HEAD" != "x" ; then )
- A couple of indentation adjustments

2023-05-27 04:21 Updated by: cazfi
  • Owner Update from cazfi to (None)
Comment

Reply To cazfi

Working on this, but it's taking a bit head scratching to figure out what you mean to happen, e.g., with local branches for which there's no similarly named branch on a remote named 'origin'

What I currently get in my work branch:

During build:

  fc_gitrev_gen.h
fatal: ambiguous argument 'origin/main-work2': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
fatal: bad revision '^'
  CC       version.lo

Running, with no changes relative to latest commit on my tree:

$ ./fcser --version
Encodings: Data=UTF-8, Local=UTF-8, Internal=UTF-8
Freeciv version 3.2.90.3-dev (origin/main-work2  HEAD b9a62d6907 (+) )

Running, with changes relative to latest commit on my tree:

$ ./fcser --version
Encodings: Data=UTF-8, Local=UTF-8, Internal=UTF-8
Freeciv version 3.2.90.3-dev (origin/main-work2  HEAD b9a62d6907 (+)  (modified))

---

I'd prefer to give this back to alain_bkr at this point.

2023-06-23 15:51 Updated by: cazfi
2023-11-10 14:52 Updated by: cazfi
2024-02-09 15:33 Updated by: cazfi

Attachment File List

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login