Ticket #3693 (testing task: fixed)

Opened 7 months ago

Last modified 8 weeks ago

Prepare for release mc-4.8.19

Reported by: zaytsev Owned by: zaytsev
Priority: major Milestone: 4.8.20
Component: adm Version: master
Keywords: Cc: egmont
Blocked By: Blocking:
Branch state: merged Votes for changeset: egmont andrew_b

Description


Attachments

mc-3641-cleanup-Wformat-signedness-common_c.patch (1.1 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-editdraw_c.patch (3.4 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-file_c.patch (1.2 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-find_c.patch (2.6 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-ftpfs_c.patch (1.1 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-hex_c.patch (1.2 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-info_c.patch (3.1 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-midnight_c.patch (1.7 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-path_c.patch (1.0 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wformat-signedness-serialize_c.patch (4.5 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-Wunsafe-loop-optimizations.patch (1.1 KB) - added by andrew_b 7 months ago.
mc-3641-cleanup-no-attribute-noreturn-utilunix.c.patch (765 bytes) - added by andrew_b 7 months ago.
mc-3641-cleanup-no-attribute-noreturn-utilvfs.c.patch (1005 bytes) - added by andrew_b 7 months ago.
mc-3641-cleanup-unreachable-code-warning-utilunix_c.patch (930 bytes) - added by andrew_b 7 months ago.
3693-Hex-patterns-fix-manual-page.patch (1.2 KB) - added by mooffie 6 months ago.
3693-mc_search_run-document-the-return-value.patch (1.1 KB) - added by mooffie 6 months ago.
This documentation is needed in order to understand the bug(s) in #3720.
3693-mc_search__cond_struct_new_regex_ci_str-add-documentation.patch (1.2 KB) - added by mooffie 5 months ago.
(This documentation is needed to better understand the patch I'll have for #3589.)
3693-mcedit-syntax.syntax-highlight-comments-preceded-by-spaces.patch (878 bytes) - added by mooffie 5 months ago.
mc-3693-dialogs.c-Cleanup-maybe-uninitialized-compiler-warni.patch (973 bytes) - added by and 5 months ago.
mc-3693-edit.c-Cleanup-unsafe-loop-optimizations-compiler-wa.patch (1.3 KB) - added by and 5 months ago.
mc-3693-editcmd.c-Cleanup-comma-compiler-warning.patch (1.5 KB) - added by and 5 months ago.
mc-3693-editcmd_dialogs.c-Cleanup-maybe-uninitialized-compil.patch (1.6 KB) - added by and 5 months ago.
mc-3693-execute.c-Cleanup-maybe-uninitialized-compiler-warni.patch (1.1 KB) - added by and 5 months ago.
mc-3693-file.c-Cleanup-maybe-uninitialized-compiler-warning.patch (1.0 KB) - added by and 5 months ago.
mc-3693-internal.c-Cleanup-format-warning.patch (981 bytes) - added by and 5 months ago.
mc-3693-lib.c-Cleanup-comma-compiler-warning.patch (1.2 KB) - added by and 5 months ago.
mc-3693-main.c-Cleanup-maybe-uninitialized-compiler-warning.patch (982 bytes) - added by and 5 months ago.
mc-3693-panel.c-Cleanup-comma-compiler-warning.patch (877 bytes) - added by and 5 months ago.
mc-3693-search.c-Cleanup-maybe-uninitialized-compiler-warnin.patch (943 bytes) - added by and 5 months ago.
mc-3693-syntax.c-Cleanup-unsafe-loop-optimizations-compiler-.patch (1.5 KB) - added by and 5 months ago.

Change History

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

Changed 7 months ago by andrew_b

comment:1 Changed 7 months ago by andrew_b

Branch: 3693_cleanup.
Initial changeset:c0c14690d8bd81ca980af265230224ead9891b17

mc-3641-cleanup-Wformat-signedness-common_c.patch​: applied.
mc-3641-cleanup-Wformat-signedness-editdraw_c.patch​​: applied.
mc-3641-cleanup-Wformat-signedness-file_c.patch​​: applied.
mc-3641-cleanup-Wformat-signedness-find_c.patch​: applied.
mc-3641-cleanup-Wformat-signedness-ftpfs_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-hex_c.patch: applied with extra small change.
mc-3641-cleanup-Wformat-signedness-info_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-midnight_c.patch: applied.
mc-3641-cleanup-Wformat-signedness-path_c.patch: another way was used.
mc-3641-cleanup-Wformat-signedness-serialize_c.patch: applied.
mc-3641-cleanup-Wunsafe-loop-optimizations.patch: applied.
mc-3641-cleanup-no-attribute-noreturn-utilunix.c.patch: applied.
mc-3641-cleanup-no-attribute-noreturn-utilvfs.c.patch: applied.
mc-3641-cleanup-unreachable-code-warning-utilunix_c.patch: applied.

comment:2 Changed 7 months ago by egmont

  • Cc egmont added

comment:3 Changed 6 months ago by andrew_b

  • Blocking 3703 added

Changed 6 months ago by mooffie

comment:4 Changed 6 months ago by andrew_b

3693-Hex-patterns-fix-manual-page.patch​: applied.

comment:5 Changed 6 months ago by mooffie

FWIW: I've read all the commits in this branch (except "Clarify startup" and "fix location of right panel", for lack of time) and they seem alright.

(As for ditching negative numbers in hex patterns (hex.c): that's a blessing (at least as long as nobody volunteers to implement reading words/dwords/etc). Besides, reading them with "%x" meant that if you typed "-10" you got -16.)

Last edited 6 months ago by mooffie (previous) (diff)

comment:6 follow-up: ↓ 7 Changed 6 months ago by mooffie

@Andrew:

re: "Clarify usage of assert":

There are two places that use g_assert() (and 3 places that use g_assert_*()).

The purpose of AC_HEADER_ASSERT is to make --disable-assert define the NDEBUG constant, which disables assert(). But to disable g_assert() one has to define the G_DISABLE_ASSERT constant.

So, it seems to me, we should choose either assert() or g_assert(), not both.

It's easier to just rename the two g_assert() we have to assert().

(One may argue that there are a few g_assert_*() functions we may covet (like g_assert_cmpint), and that for the sake of stylistic uniformity we should therefore prefer g_assert() to assert(). But these g_assert_*() bunch can't be disabled, so we may treat them as a category of their own.)

Not that I care too much about any of this. It's just that I'm about to propose a patch in which I was planning to use g_assert() / assert() and I don't want to get some flak ;-)

comment:7 in reply to: ↑ 6 Changed 6 months ago by andrew_b

Replying to mooffie:

So, it seems to me, we should choose either assert() or g_assert(), not both.

Indeed.

I prefer g_assert() and I've added the commit.

comment:8 Changed 6 months ago by zaytsev

@andrew_b: just a nitpick, the plural of assert is not "Asserts:", but "Assertions:".

comment:9 Changed 6 months ago by zaytsev

Here too: "Define to disable asserts" -> "Define to disable assertions".

comment:10 Changed 6 months ago by andrew_b

Rebased to current master: [e036c1fb12757dc9d258862736054c33313a51d5].

comment:11 Changed 6 months ago by andrew_b

  • Branch state changed from no branch to on review

Changed 6 months ago by mooffie

This documentation is needed in order to understand the bug(s) in #3720.

comment:12 Changed 6 months ago by andrew_b

3693-mc_search_run-document-the-return-value.patch​: applied.

Rebased to current master: [645e88d209a520e6c83eeced710b9376f878c8f8].

comment:13 Changed 5 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:14 Changed 5 months ago by andrew_b

  • Votes for changeset changed from andrew_b to committed-master
  • Branch state changed from approved to merged

Merged to master: [24a16003a8c01faaeb4e907e674c7bddb36e1424].

git log --pretty=oneline da538f0..24a1600

comment:15 Changed 5 months ago by andrew_b

  • Blocking 3703 removed

comment:16 Changed 5 months ago by andrew_b

  • Votes for changeset committed-master deleted
  • Branch state changed from merged to no branch

Next series: 3693_cleanup
Initial changeset:9231e10791fa313a04afcf7eb78e671ea0ac3d47

Last edited 5 months ago by andrew_b (previous) (diff)

Changed 5 months ago by mooffie

(This documentation is needed to better understand the patch I'll have for #3589.)

comment:17 Changed 5 months ago by andrew_b

3693-mc_search__cond_struct_new_regex_ci_str-add-documentation.patch: applied. Thanks!

Last edited 5 months ago by andrew_b (previous) (diff)

comment:18 Changed 5 months ago by mooffie

The following patch is too trivial for a separate ticket. It improves the highighting of comments in some syntax files.

# list all syntax files with comments not on 1st column:

$ grep -P '^\s+#' -l *

as.syntax
ebuild.syntax
jal.syntax
js.syntax
lkr.syntax
lua.syntax
m4.syntax
yaml.syntax

(The patch doesn't add support for comments preceded by TABs, though. But that's no biggie.)

comment:19 Changed 5 months ago by andrew_b

3693-mcedit-syntax.syntax-highlight-comments-preceded-by-spaces.patch: applied.

Changed 5 months ago by and

comment:20 follow-up: ↓ 26 Changed 5 months ago by andrew_b

mc-3693-dialogs.c-Cleanup-maybe-uninitialized-compiler-warni.patch​: [1]
mc-3693-edit.c-Cleanup-unsafe-loop-optimizations-compiler-wa.patch​: [2]
mc-3693-editcmd.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-editcmd_dialogs.c-Cleanup-maybe-uninitialized-compil.patch​: [1]
mc-3693-execute.c-Cleanup-maybe-uninitialized-compiler-warni.patch​: [1]
mc-3693-file.c-Cleanup-maybe-uninitialized-compiler-warning.patch​: [1]
mc-3693-internal.c-Cleanup-format-warning.patch​: applied.
mc-3693-lib.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-main.c-Cleanup-maybe-uninitialized-compiler-warning.patch: [1]
mc-3693-panel.c-Cleanup-comma-compiler-warning.patch​: [3]
mc-3693-search.c-Cleanup-maybe-uninitialized-compiler-warnin.patch​: [1]
mc-3693-syntax.c-Cleanup-unsafe-loop-optimizations-compiler-.patch: [2]

[1] squashed in the single commit.
[2] I can't see something wrong in these loops.
[3] squashed in the single commit.

Last edited 5 months ago by andrew_b (previous) (diff)

comment:21 Changed 5 months ago by andrew_b

We have strange build error:

  CC     file.lo
../../../../src/vfs/sftpfs/file.c: In function ‘sftpfs_open_file’:
../../../../src/vfs/sftpfs/file.c:156:16: error: missing initializer [-Werror=missing-field-initializers]
../../../../src/vfs/sftpfs/file.c:156:16: error: (near initialization for ‘file_info.st_ino’) [-Werror=missing-field-initializers]

gcc-4.6.3 is used.

The code is following:

155     {
156         struct stat file_info = { 0 };
157 
158         if (sftpfs_fstat (file_handler, &file_info, mcerror) == 0)
159             libssh2_sftp_seek64 (file_handler_data->handle, file_info.st_size);
160     }

What's wrong here?

My local build with gcc-5.3.1 is finished successfully.

comment:22 follow-up: ↓ 23 Changed 5 months ago by zaytsev-work

Try struct stat file_info = { }; ? I think that it complains, because if you specify the first 0, then it thinks that you should you should go all the way and specify all of them, but this annoying behaviour for this warning was changed in later GCCs.

comment:23 in reply to: ↑ 22 Changed 5 months ago by andrew_b

Replying to zaytsev-work:

Try struct stat file_info = { }; ?

This was in the original patch. The result was same:

  CC     file.lo
../../../../src/vfs/sftpfs/file.c: In function ‘sftpfs_open_file’:
../../../../src/vfs/sftpfs/file.c:156:16: error: missing initializer [-Werror=missing-field-initializers]
../../../../src/vfs/sftpfs/file.c:156:16: error: (near initialization for ‘file_info.st_dev’) [-Werror=missing-field-initializers]

comment:24 follow-up: ↓ 25 Changed 5 months ago by zaytsev-work

Oh, sorry, stupid me, struct stat file_info = { }; isn't even valid C code according to the standard (which can be witnessed via -pedantic):

http://stackoverflow.com/questions/17589533/is-an-empty-initializer-list-valid-c-code

It's only valid in C++, so your change to struct stat file_info = { 0 }; is absolutely correct.

Anyways, I have found the GCC bug report regarding { 0 } in C, and, unfortunately, it only made it in GCC 4.7.x:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36750

However, it seems a workaround for GCC 4.6.x is possible:

http://stackoverflow.com/a/27461062

struct stat file_info = { .st_dev = 0 };

Maybe this will work? The only other thing that comes to mind is to mess with pragmas, but it smells so badly that I wouldn't consider this as an option... :-(

comment:25 in reply to: ↑ 24 Changed 5 months ago by andrew_b

Replying to zaytsev-work:

Maybe this will work?

Yes, it works. Thanks!

comment:26 in reply to: ↑ 20 ; follow-up: ↓ 27 Changed 5 months ago by and

Replying to andrew_b:

mc-3693-edit.c-Cleanup-unsafe-loop-optimizations-compiler-wa.patch​: [2]

[2] I can't see something wrong in these loops.

GCC complains about unsafe loop optimization when "... <= VALUE" found,
because internal comparing works like "... < (VALUE+1)" and possible VALUE=MAX_INT
will overflow, hence unsafe loop optimization possible.

Last section at
https://stackoverflow.com/questions/2982507/what-is-the-explanation-for-warning-assuming-that-the-loop-is-not-infinite

comment:27 in reply to: ↑ 26 Changed 5 months ago by andrew_b

Replying to and:

Last section at
https://stackoverflow.com/questions/2982507/what-is-the-explanation-for-warning-assuming-that-the-loop-is-not-infinite

I found this webpage too. I think GCC is some paranoiac here.

comment:28 Changed 4 months ago by andrew_b

  • Branch state changed from no branch to on review

comment:29 Changed 3 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:30 Changed 3 months ago by andrew_b

  • Votes for changeset changed from andrew_b to committed-master
  • Branch state changed from approved to merged

Merged to master: [9845fa6ce542c5c45ab9a86140052dd3ded03655].

git log --pretty=oneline 43d7fce..9845fa6

comment:32 follow-up: ↓ 33 Changed 2 months ago by zaytsev

Andrew, are you fine with committing the patch above?

comment:33 in reply to: ↑ 32 Changed 2 months ago by mooffie

Replying to zaytsev:

Andrew, are you fine with committing the patch above?


My patch? No need to. It was eventually committed in #3760 (I posted it here at a time it seemed my solution for #3760 would be rejected). I've just deleted my comment+patch here.

comment:34 Changed 2 months ago by zaytsev

Ah, ok thanks, sorry, I've got completely confused in the end.

comment:35 Changed 2 months ago by zaytsev

It seems that all is calm, so I'm planning to pull in latest translations and make the release this weekend, probably on Sunday, unless there are objections to this plan.

comment:36 Changed 2 months ago by egmont

Sounds great!

comment:37 Changed 8 weeks ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev
  • Votes for changeset committed-master deleted
  • Branch state changed from merged to no branch

download PO-translations from Transifex.net

done

store translations in git repo

done

download the hint translations from Transifex.net

done; nothing new

store translations in git repo

skip

create new NEWS wiki page for next version with empty template

done; NEWS-4.8.20

add content of current NEWS wiki page to the doc/NEWS file

done

create new tag in git by command

done

new version in Trac

done

new milestone in Trac

done

create tar.(bz2|xz) package files

done

make checksums for archives

done

upload source packages and checksums to the special upload area

done

developers should download tarballs, verify checksums, compile and install locally; if everything is ok, then developers vote for the ticket

http://midnight-commander.org/nopaste/tarball/

please!

comment:38 Changed 8 weeks ago by egmont

I hardly ever remember to run "make check", but when I do:

FAIL: utilunix__my_system_fork_child_shell
FAIL: utilunix__my_system_fork_child

The corresponding log files contain this:

Running suite(s): /lib/utilunix


fork () = -1
FAIL utilunix__my_system_fork_child (exit status: 1)

The tests succeed for me with the 4.8.18 tarball.

comment:39 Changed 8 weeks ago by zaytsev

Unfortunately, you are right, good catch! I have reproduced the problem on my Fedora VM, but I have no idea what that is. These tests are failing permanently on Travis, which I why I assumed it has something to do with Travis environment proper. I've started a bisection run between 4.8.19 and 4.8.18 to narrow down the culprit, but it's progressing very slowly.

comment:40 Changed 8 weeks ago by egmont

I'm also running git bisect, it's almost finished :) You can stop yours.

Just wondering, is "make check" not executed in the continuous build, and as part of the release process? If so, at least the release process documentation should be extended with this entry.

comment:41 Changed 8 weeks ago by egmont

comment:42 follow-up: ↓ 43 Changed 8 weeks ago by zaytsev

I'm also running git bisect, it's almost finished :) You can stop yours.

I've got ~7 commits to go, so I guess I'll leave it running anyways just to see if I can match your results ;-)

Just wondering, is "make check" not executed in the continuous build

It definitively is. The problem is, some tests fail on Travis for mysterious reasons and Andrew couldn't reproduce it, and others are failing for reasons that I understood, but was not able to find time to fix so far. Therefore, I had to make Travis builds only fail when the build itself fails, not upon test failures, which obviously make real problems easy to overlook... :-( but it's better than not running them at all, at least I sometimes inspect the logs visually.

as part of the release process? If so, at least the release process documentation should be extended with this entry.

Actually, implicitly it is, I just skipped it because it all takes so long on my underpowered laptop :-/ but you are right, this is not good. I have added it to the guidelines.

comment:43 in reply to: ↑ 42 Changed 8 weeks ago by egmont

Replying to zaytsev:

I've got ~7 commits to go, so I guess I'll leave it running anyways just to see if I can match your results ;-)

Mandatory link: #2252 :P

Actually, implicitly it is, I just skipped it because it all takes so long on my underpowered laptop :-/ but you are right, this is not good. I have added it to the guidelines.

Thanks!

comment:44 Changed 8 weeks ago by zaytsev

b91ab44b43570ebebe58627efedcb2adb4066023 is the first bad commit

Confirm b91ab44b43570ebebe58627efedcb2adb4066023 ! Good thing the commit is so small, that's why we want them to be like this --- upon problems bisect points you directly to where the trouble is. But I'm confused as to why marking noreturn function as such makes the test fail. Need to have a closer look later :-/

comment:45 follow-up: ↓ 47 Changed 8 weeks ago by zaytsev

Mandatory link: #2252 :P

Well, the way I do bisects is as follows, and this is already a shortcut, so in this particular case #2252 is irrelevant

[zaytsev@localhost mc]$ git bisect run sh -c '(./autogen.sh && rm -rf build && mkdir -p build && cd build && ../configure && make check)'

comment:46 Changed 8 weeks ago by zaytsev

Ha, maybe I should have read the comment why this function was introduced in the first place:

 * The _exit() function has gcc's attribute 'noreturn', and this is reason why we can't
 * mock the call.

So apparently the warning should have been suppressed instead!

comment:47 in reply to: ↑ 45 Changed 8 weeks ago by egmont

Replying to zaytsev:

[zaytsev@localhost mc]$ git bisect run sh -c '(./autogen.sh && rm -rf build && mkdir -p build && cd build && ../configure && make check)'

I didn't know about this possibility, I'm always doing it "manually".

But... apparently this construct cannot differentiate between failed tests (where you'd type "git bisect bad" and failed builds ("git bisect skip").

Anyway, it's always good to learn new practices :)

Last edited 8 weeks ago by egmont (previous) (diff)

comment:48 Changed 8 weeks ago by zaytsev

I didn't know about this possibility, I'm always doing it "manually".

w00t!!!11 you are a man of great patience... I always start bisects in the background if results are easily verifiable by script and do other stuff while they are running.

But... apparently this construct cannot differentiate between failed tests (where you'd type "git bisect bad" and failed builds ("git bisect skip").

This is just because I know that there will be no failed builds in master, but otherwise I would add make || exit 125 to tell git to skip the commit if the build fails.

Okay, as to the commit, I propose a revert + a follow up commit to clarify that the comment function should not be decorated.

Then I will make a new tag and rebuild the tarballs. I haven't pushed the tag out yet.

comment:49 Changed 8 weeks ago by zaytsev

Re-built and re-uploaded the tarballs.

comment:50 Changed 8 weeks ago by zaytsev

And again, today is definitively not my day :-(

comment:51 follow-up: ↓ 52 Changed 8 weeks ago by egmont

What was wrong with the comment on the same line?

comment:52 in reply to: ↑ 51 Changed 8 weeks ago by zaytsev

Replying to egmont:

What was wrong with the comment on the same line?

It confused GNU Indent and as it tried to reformat the thing, it was breaking the formatting completely, but somehow I didn't notice it until it was too late :-/ Oh well, the bottom line, it likes it when it's on a separate line and it seems that all is good now.

comment:53 Changed 8 weeks ago by egmont

  • Votes for changeset set to egmont

comment:54 Changed 8 weeks ago by andrew_b

  • Votes for changeset changed from egmont to egmont andrew_b
  • Branch state changed from no branch to approved

comment:55 Changed 8 weeks ago by zaytsev

  • Status changed from accepted to testing
  • Resolution set to fixed
  • Branch state changed from approved to merged

upload source packages and checksums

done

run command: ...

done

update Wiki start page with latest release number

done

write an announcement: list user visible changes (bugs and features)

done

close current milestone

in a moment

create new ticket (type=task, component=adm) for the next release

done; #3780

close ticket for release

done

Note: See TracTickets for help on using tickets.