Ticket #4524 (closed task: fixed)
Prepare for release mc-4.8.32
Reported by: | zaytsev | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | 4.8.32 |
Component: | adm | Version: | master |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Branch state: | merged | Votes for changeset: |
Description
Attachments
Change History
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 11 months ago by andrew_b
Replying to zaytsev:
- https://github.com/MidnightCommander/mc/pull/195 - C/C++ syntax update
Keywords taken from: https://en.cppreference.com/w/c/keyword https://en.cppreference.com/w/cpp/keyword
C++ keywords should be moved to cxx.syntax.
comment:4 Changed 11 months ago by andrew_b
Branch: 4524_cleanup
Initial changeset:aca0f2275585fe8f0d68c7b4054990824cb42139
comment:5 Changed 9 months ago by andrew_b
Merged to master: [cfedd6598c415ed0fadbd37ccf6fc933d3058134].
git log --oneline 7b3c427c8..cfedd6598
comment:6 Changed 8 months ago by andrew_b
Branch: 4524_cleanup
Initial changeset:9bde3cedced5b9ece23d5eb8ba5b26725c467f57
comment:8 Changed 8 months ago by andrew_b
-exec lsdl
$ lsdl bash: lsdl: command not found
Yet another extra dependency for mc is not very well.
Proper ticket is #3570.
comment:10 follow-up: ↓ 11 Changed 8 months ago by andrew_b
I think it should be a new ticket.
comment:11 in reply to: ↑ 10 Changed 7 months ago by zaytsev
Changed 7 months ago by zaytsev
- Attachment 0001-m4-add-Werror-when-checking-for-compiler-flags.patch added
comment:12 Changed 7 months ago by zaytsev
I have tried to build mc on macOS and got bombarded with bogus flags warnings. Researched it and apparently at some point clang, which is symlinked as gcc on macOS started accepting random flags without changing the return code, but outputting tons of spammy warnings. Of course, this effectively broke compiler flags checking. I have found a solution by Mesa people - just add -Werror and the checker will work correctly again. Will see if it can be reported to the autotools archive...
comment:13 Changed 7 months ago by zaytsev
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-fix-build-on-macOS-with-libssh2-present-via.patch added
comment:14 follow-up: ↓ 20 Changed 7 months ago by zaytsev
mc would not build from source on macOS because libssh2 path was missing from CPPFLAGS. See attached patch.
Now it builds, but still there are tons of warnings from clang. Mostly they are coming from -Wassign-enum, like this one, but there are also other systematic issues:
../../../lib/widget/input.c:962:19: warning: integer constant not in range of enumerated type 'widget_options_t' [-Wassign-enum] w->options |= WOP_SELECTABLE | WOP_IS_INPUT | WOP_WANT_CURSOR; ^
How do we proceed about those? Do you have latest clang and interest to have a look yourself? Should I try to find groups so that we agree on how to fix them? I think it's a bit too much for me. Maybe if we know what we want we can ask and@ for help :)
comment:15 Changed 7 months ago by zaytsev
This one is probably tracked here: https://github.com/llvm/llvm-project/issues/20574
comment:16 Changed 7 months ago by zaytsev
Another large swath of warnings is due to fallthroughs:
../../../src/editor/editwidget.c:1137:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] case MSG_MOUSE_UP: ^ ../../../src/editor/editwidget.c:1137:5: note: insert '__attribute__((fallthrough));' to silence this warning case MSG_MOUSE_UP: ^ __attribute__((fallthrough)); ../../../src/editor/editwidget.c:1137:5: note: insert 'break;' to avoid fall-through case MSG_MOUSE_UP: ^ break;
Somehow HAVE_FUNC_ATTRIBUTE_FALLTHROUGH is undefined, so detection is not working correctly. Need to check how to fix that.
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-update-ax_gcc_func_attribute-to-fix-fallthr.patch added
comment:17 Changed 7 months ago by zaytsev
Need to check how to fix that.
Attached patch fixes fallthrough detection for clang.
Changed 7 months ago by zaytsev
- Attachment 0001-fixup-m4-add-Werror-when-checking-for-compiler-flags.patch added
comment:18 follow-up: ↓ 19 Changed 7 months ago by zaytsev
The latest version of GNU indent sticks pointers either to the left or to the right. Can we reformat master to not have huge diffs all the time?
-mc_fhl_is_device_block (const file_entry_t * fe) +mc_fhl_is_device_block (const file_entry_t *fe)
comment:19 in reply to: ↑ 18 Changed 7 months ago by andrew_b
Replying to zaytsev:
The latest version of GNU indent sticks pointers either to the left or to the right.
It depends on type. For standard types (int, void) and types with struct and union keywords
int foo (int *a); int bar (void *b); int baz (struct st *s);
For other types
mc_fhl_is_device_block (const file_entry_t * fe)
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-prefer-gnu-indent-gindent-to-bsd-indent-if-.patch added
comment:20 in reply to: ↑ 14 Changed 7 months ago by andrew_b
Replying to zaytsev:
mc would not build from source on macOS because libssh2 path was missing from CPPFLAGS. See attached patch.
AM_CPPFLAGS = $(GLIB_CFLAGS) -I$(top_srcdir) $(LIBSSH_CFLAGS)
is in src/vfs/sftpfs/Makefile.am. Is it not enough?
Now it builds, but still there are tons of warnings from clang. Mostly they are coming from -Wassign-enum, like this one, but there are also other systematic issues:
../../../lib/widget/input.c:962:19: warning: integer constant not in range of enumerated type 'widget_options_t' [-Wassign-enum] w->options |= WOP_SELECTABLE | WOP_IS_INPUT | WOP_WANT_CURSOR; ^How do we proceed about those?
Only use individual constants instead of enum.
comment:21 Changed 7 months ago by zaytsev
is in src/vfs/sftpfs/Makefile.am. Is it not enough?
No, it is not enough. Check src/textconf.[ch] - it's also used there.
comment:22 Changed 7 months ago by zaytsev
Only use individual constants instead of enum.
Ok, then I suggest we wait until clang people make up their mind. It seems that they have reacted to my issue and want to have a look. Maybe we don't need to do anything in the end. For now I have locally disabled -Wassign-enum.
comment:23 follow-up: ↓ 27 Changed 7 months ago by zaytsev
It depends on type. For standard types (int, void) and types with struct and union keywords
So for you the latest version of indent doesn't generate a diff in the repository?! With my latest patch GNU indent is used, and I'm getting a huge diff. All pointers are right-aligned irrespectively of the type:
zaytsev@Yurys-MBP src % gindent --version GNU indent 2.2.13
Changed 7 months ago by zaytsev
- Attachment 0001-clang-fix-Wimplicit-fallthrough-warning.patch added
Changed 7 months ago by zaytsev
- Attachment 0001-clang-fix-Wconditional-uninitialized-warnings.patch added
comment:24 Changed 7 months ago by zaytsev
I think these are all legit clang warnings... remaining don't come from us or are false positives.
comment:25 Changed 7 months ago by andrew_b
0001-clang-fix-Wconstant-conversion-warning.patch
Why umask() is applied here?
0001-clang-fix-Wimplicit-fallthrough-warning.patch
I think, break should be here.
comment:26 Changed 7 months ago by zaytsev
Why umask() is applied here?
Oh, thought this was a cast, but it's system call o_O I've stolen the code from coreutils, and we also have similar patterns in our code. I guess correct would be ~ (mode_t) 0, would you change that?
I think, break should be here.
I didn't know what you will like more. Both are fine with me.
comment:27 in reply to: ↑ 23 Changed 7 months ago by andrew_b
Replying to zaytsev:
So for you the latest version of indent doesn't generate a diff in the repository?! With my latest patch GNU indent is used, and I'm getting a huge diff. All pointers are right-aligned irrespectively of the type:
Yes, the latest indent generates some different result.
I'll make indent before merge 4524_cleanup to master.
comment:28 follow-up: ↓ 29 Changed 7 months ago by zaytsev
Do you have any specific plans on when to merge cleanup for the next time? If you are not up to anything special and you agree with my patches (with your changes), it would be great if you could take them, make indent and merge it.
I am now working on fixing the nanosecond mess on different platforms in a different branch, but it's based on my macOS fixes, because otherwise I can't work on my laptop.
After that I'm also planning to have a look at the long standing issue of moving the CI from Travis to GitHub Actions. If macOS fixes are in tree, then I will be able to add macOS / clang as a builder, so that we will finally get some live BSD coverage for our changes....
comment:29 in reply to: ↑ 28 Changed 7 months ago by andrew_b
- Branch state changed from no branch to merged
Replying to zaytsev:
it would be great if you could take them, make indent and merge it.
Done.
Merged to master: [4139bb82a73d49ddae6daf91419aef0b439d8db2].
git log --oneline 942869713..4139bb82a
comment:30 follow-up: ↓ 32 Changed 7 months ago by zaytsev
Some interesting warnings on AIX:
../../../src/filemanager/cmd.c: In function 'compare_files': ../../../src/filemanager/cmd.c:224:36: warning: 'memcmp' specified size 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overflow=] 224 | result = (n1 != n2) || memcmp (buf1, buf2, n1); | ^~~~~~~~~~~~~~~~~~~~~~~
Not sure if it makes sense.
Changed 7 months ago by zaytsev
- Attachment Screenshot 2024-06-02 at 23.24.56.png added
mc on AIX 7.3
comment:31 Changed 7 months ago by zaytsev
File sizes are kind of... a bit off on my screenshot, even for AIX, billions of gigabytes is a bit too much:
bash-5.1$ ls -als total 104 4 drwxr-xr-x 8 zaytsev usr 4096 Jun 2 21:21 . 64 drwxr-xr-x 1026 root system 61440 Jun 2 11:03 .. 12 -rw------- 1 zaytsev usr 11837 Jun 2 21:04 .bash_history 0 drwx------ 3 zaytsev usr 256 Jun 2 21:21 .cache 0 drwx------ 3 zaytsev usr 256 Jun 2 21:21 .config 0 drwx------ 3 zaytsev usr 256 Jun 2 21:21 .local 4 -rwxr----- 1 zaytsev usr 370 Jun 2 21:03 .profile 0 drwx------ 2 zaytsev usr 256 May 31 10:08 .ssh 16 -rw------- 1 zaytsev usr 15417 Jun 2 21:03 .viminfo 4 -rw-r--r-- 1 zaytsev usr 284 Jun 2 19:02 .wget-hsts 0 drwxr-xr-x 7 zaytsev usr 256 Jun 2 21:21 opt 0 drwxr-xr-x 3 zaytsev usr 256 Jun 1 19:26 src
comment:32 in reply to: ↑ 30 Changed 7 months ago by andrew_b
Replying to zaytsev:
Some interesting warnings on AIX:
../../../src/filemanager/cmd.c: In function 'compare_files': ../../../src/filemanager/cmd.c:224:36: warning: 'memcmp' specified size 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overflow=] 224 | result = (n1 != n2) || memcmp (buf1, buf2, n1); | ^~~~~~~~~~~~~~~~~~~~~~~
2147483647 = 0x7FFFFFFF, a INT32_MAX.
4294967295 = 0xFFFFFFFF = -1, UINT32_MAX
Probably, initialization of n1 and n2 is needed:
int n1 = 0, n2 = 0;
or even change of type:
ssize_t n1 = 0, n2 = 0;
comment:33 Changed 7 months ago by zaytsev
Probably, initialization of n1 and n2 is needed
Initialization and change to ssize_t didn't help :(
But I've noticed something much more scary, even worse than crazy file sizes. The editor opens, but the screen is completely empty. The viewer works just fine...
comment:34 Changed 7 months ago by andrew_b
What branch are you testing on AIX?
comment:35 follow-up: ↓ 36 Changed 7 months ago by zaytsev
4542_nanoseconds_cleanup, which is now rebased on cleanup you merged to master. Master does not build, I made build fixes in the branch 4542_nanoseconds_cleanup yesterday, which you wanted me to take to master later.
comment:36 in reply to: ↑ 35 Changed 7 months ago by andrew_b
Replying to zaytsev:
4542_nanoseconds_cleanup
I think wrong file sizes are because of absent config.h in local.c. Some macros (such as USE_LARGEFILE64 and USE_FILE_OFFSET64) used in sys/stat.h are undefined when sys/stat.h is included via other headers.
comment:37 Changed 7 months ago by zaytsev
I think wrong file sizes are because of absent config.h in local.c.
Hey Andrew, you are a genius. I have rebuilt with fixed branch, and now file sizes and editor are working! Yay! Now you can actually work and develop on this thing.
There is another problem though. Somehow ./configure pulls in libtermcap even if it can't find it, but this library is not present on AIX:
checking if S-Lang uses termcap... yes ... configure:20395: using S-Lang screen library with termcap configure:20400: checking for tgoto in -ltermcap configure:20429: gcc -o conftest -g -O2 -I/home/zaytsev/opt/slang/include conftest.c -ltermcap >&5 collect2: fatal error: library libtermcap not found compilation terminated.
If I manually remove -ltermcap from all Makefiles, then mc compiles, links and works. So it is apparently not necessary. Or maybe it is and something is subtly broken, but I'm not noticing it? The system has libcurses - I saw some posts on the Internet, that this can be used if libtermcap is not found.
Can we somehow fix it so that editing the makefiles in not necessary?
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-fix-broken-detection-for-termcap-libraries-.patch added
comment:38 Changed 7 months ago by zaytsev
I think that now I understand the reason - our code adds -ltermcap always - even if it's not found, which is wrong. I changed the detection to try several different libraries and fail if nothing works, instead of silently adding wrong linker flags. Can I take it to master? Then AIX becomes first class platform after those fixes :)
Maybe I should try Solaris now... oh wait..
comment:39 follow-up: ↓ 40 Changed 7 months ago by zaytsev
../configure \ --prefix="$(pwd)/install" \ --disable-shared \ --disable-static \ --disable-maintainer-mode \ --disable-largefile \ --disable-nls \ --disable-rpath \ --disable-charset \ --disable-mclib \ --disable-assert \ --disable-aspell \ --disable-background \ --disable-vfs \ --disable-doxygen-doc \ --without-x \ --without-mmap \ --without-gpm-mouse \ --without-internal-edit \ --without-diff-viewer \ --without-subshell \ --enable-tests \ --enable-werror
../../../src/viewer/datasource.c: In function ‘mcview_set_byte’: ../../../src/viewer/datasource.c:270:38: error: unused parameter ‘offset’ [-Werror=unused-parameter] 270 | mcview_set_byte (WView * view, off_t offset, byte b) | ~~~~~~^~~~~~
Also, make indent now adds an extra /* at the top of one file:
diff --git a/lib/vfs/parse_ls_vga.c b/lib/vfs/parse_ls_vga.c index 3b179ebda..09bda6f1c 100644 --- a/lib/vfs/parse_ls_vga.c +++ b/lib/vfs/parse_ls_vga.c @@ -1,3 +1,4 @@ +/* /* Routines for parsing output from the 'ls' command.
comment:40 in reply to: ↑ 39 Changed 7 months ago by andrew_b
Replying to zaytsev:
../../../src/viewer/datasource.c: In function ‘mcview_set_byte’: ../../../src/viewer/datasource.c:270:38: error: unused parameter ‘offset’ [-Werror=unused-parameter] 270 | mcview_set_byte (WView * view, off_t offset, byte b) | ~~~~~~^~~~~~
Likely due to --disable-assert. Just add
(void) offset;
in mcview_set_byte().
Also, make indent now adds an extra /* at the top of one file:
diff --git a/lib/vfs/parse_ls_vga.c b/lib/vfs/parse_ls_vga.c index 3b179ebda..09bda6f1c 100644 --- a/lib/vfs/parse_ls_vga.c +++ b/lib/vfs/parse_ls_vga.c @@ -1,3 +1,4 @@ +/* /* Routines for parsing output from the 'ls' command.
In my case, extra /* is added in lib/util.h. Moreover,
indent: lib/util.c:469: Error:Unmatched 'else' indent: lib/util.c:471: Error:Unmatched #else indent: lib/util.c:475: Error:Unmatched #endif indent: lib/util.c:522: Error:Stmt nesting error.
comment:41 Changed 7 months ago by zaytsev
Likely due to --disable-assert. Just add
Thank you, this worked! Here is my idea of what GitHub CI could look like. Do you have any strong opinions? My idea is to test the typical case, the nurses & pcre, then everything disabled. Should give a reasonably good coverage. Next I would add macOS on ARM, and we could also use old Ubuntu 20.04 to test systems that are a bit dated. More doesn't come to mind.
https://github.com/MidnightCommander/mc/actions/runs/9355864284/job/25751986426
That GNU indent is behaving erratically is annoying. We could switch to something less crazy like clang-format, but I don't know what would be your opinion on that. So far in my experience clang-format was the only really stable option. But I would really want to just set a watcher to formatter and forget about it, and make sure CI checks it too...
comment:42 Changed 7 months ago by zaytsev
The tests are failing in minimal configurations:
https://github.com/MidnightCommander/mc/actions/runs/9363598464/job/25774736754
PASS: library_independ PASS: mc_build_filename PASS: name_quote PASS: serialize PASS: utilunix__mc_pstream_get_string PASS: utilunix__my_system_fork_fail FAIL: utilunix__my_system_fork_child_shell FAIL: utilunix__my_system_fork_child PASS: x_basename ============================================================================ Testsuite summary for "/lib" ============================================================================ # TOTAL: 9 # PASS: 7 # SKIP: 0 # XFAIL: 0 # FAIL: 2 # XPASS: 0 # ERROR: 0
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-make-it-possible-to-specify-aspell-prefix-f.patch added
comment:43 Changed 7 months ago by zaytsev
Another patch that I would like to take to master to specify custom prefix to aspell since it doesn't use pkg-config - this is for example useful on macOS, instead of having to fiddle with compile flags.
comment:44 follow-up: ↓ 45 Changed 7 months ago by zaytsev
Tests fail on macOS :(
Making check in . /Applications/Xcode_15.0.1.app/Contents/Developer/usr/bin/make library_independ mc_build_filename name_quote serialize utilunix__mc_pstream_get_string utilunix__my_system_fork_fail utilunix__my_system_fork_child_shell utilunix__my_system_fork_child x_basename mc_realpath CC library_independ.o CCLD library_independ ld: unknown options: -z clang: error: linker command failed with exit code 1 (use -v to see invocation)
comment:45 in reply to: ↑ 44 Changed 7 months ago by andrew_b
Replying to zaytsev:
ld: unknown options: -z clang: error: linker command failed with exit code 1 (use -v to see invocation)
m4.include/mc-tests.m4
51 # on cygwin, the linker does not accept the "-z" option 52 case $host_os in 53 cygwin*) 54 TESTS_LDFLAGS="-Wl,--allow-multiple-definition" 55 ;; 56 *) 57 TESTS_LDFLAGS="-Wl,-z,muldefs" 58 ;; 59 esac
From other hand, to avoid -z flag at all system routines that are redefined in test should be called via wrappers. But this is a task for another ticket.
comment:46 follow-ups: ↓ 47 ↓ 50 Changed 7 months ago by zaytsev
m4.include/mc-tests.m4
Hmmm, maybe I can have a look at checking with autotools what linker flags are allowed and using those to cook up a patch, thank you!
Are you fine with the termcap and aspell patches? Would you have look at failing tests on Linux or do you have a tip for me what could be the problem?
comment:47 in reply to: ↑ 46 Changed 7 months ago by andrew_b
Replying to zaytsev:
Are you fine with the termcap and aspell patches?
Sorry, I haven't tested them yet.
Would you have look at failing tests on Linux or do you have a tip for me what could be the problem?
FAIL: utilunix__my_system_fork_child_shell FAIL: utilunix__my_system_fork_child
Can't say what is going on. On my linux all tests are passed.
comment:48 Changed 7 months ago by zaytsev
Do the tests also pass on your machine with
../configure \ --prefix="$(pwd)/install" \ --disable-shared \ --disable-static \ --disable-maintainer-mode \ --disable-largefile \ --disable-nls \ --disable-rpath \ --disable-charset \ --disable-mclib \ --disable-assert \ --disable-aspell \ --disable-background \ --disable-vfs \ --disable-doxygen-doc \ --without-x \ --without-mmap \ --without-gpm-mouse \ --without-internal-edit \ --without-diff-viewer \ --without-subshell \ --enable-tests \ --enable-werror
???
comment:49 Changed 7 months ago by andrew_b
No, they don't. But they do in case of compile with debugging info
make CFLAGS="-g -ggdb3"
comment:50 in reply to: ↑ 46 Changed 7 months ago by andrew_b
Replying to zaytsev:
Are you fine with the termcap and aspell patches?
Yes, I am.
configure:18574: checking if S-Lang uses termcap configure:18595: gcc -o conftest -g -O2 -I/usr/include/slang conftest.c -lslang >&5 configure:18595: $? = 0 configure:18606: result: no
It would be great to add some help about --enable-aspell argument in m4-file as well as in doc/INSTALL.
Changed 7 months ago by zaytsev
- Attachment 0001-buildsys-make-it-possible-to-specify-aspell-prefix-f.2.patch added
comment:51 Changed 7 months ago by zaytsev
Yes, I am.
Thank you for your helpful comments. Termcap patch is committed to master and nano-branch rebased. I attach the new version of the aspell patch. Changed prefix to DIR for consistency with screen selection and added comments to that effect. I wonder why we wanted to make it dlopenable, but whatever :) I think that if someone puts it in a local checkout, he will have to make sure he sets RPATH or loader path appropriately. For non-standard opt-trees like homebrew it will simply work.
comment:52 follow-up: ↓ 54 Changed 7 months ago by zaytsev
From other hand, to avoid -z flag at all system routines that are redefined in test should be called via wrappers. But this is a task for another ticket.
So I've had a look at it and I think I understand the problem a bit better now. The tests mock some functions by adding functions with the same name to the test translation module. After that there are two definitions of the function, and if you enable -z,muldefs or --allow-multiple-definition depending on the linker, then both end up into the binary. After that the dynamic loader takes the last one (?) and hopefully everything works.
The problem with that is that on macOS (I can't say for other systems, but surely AIX and Solaris will be fun as well) both of those switches do not exist. There used to be an -m switch to do the same, but very long time ago, Apple deprecated it, and it no longer works. So on Apple systems there is no way to achieve binaries with multiply defined symbols using the standard system linker. Maybe it's possible with other linkers (apparently ld64 supports macOS, but I didn't check).
This leaves us with the question of what to do. One thing that I have discovered is that what works on macOS is to decorate the functions that you want to mock with __attribute__((weak)). If there is no second definition during the linking, then it makes no difference, but if there is, then the one that doesn't have the attribute takes precedence. This way I was able to get the tests to (mostly) run on macOS with the standard system linker.
Now, of course, you probably don't want this in your standard build, so one would have to make it conditional upon --enable-tests, but even then, I'm not sure if it's a cool idea to have mc binary produced with weak symbols if --enable-tests is given. I would guess that in the end, if during the linking no overriding symbols are found the binary will be identical, but I wouldn't venture to claim that at the moment I understand this completely. Do you have more knowledge on whether weak symbols remain weak after linking or this concept only makes sense for translation units before linking?
Another point is that all mockable functions will have to be marked. In my personal opinion it's better than allowing uncontrolled muldefs, but I don't know what your opinion is on that.
The alternative to __attribute__((weak)) is to replace all functions with function pointers, but that somehow feels even worse to me.
I would like to have your opinion on that before I continue working on this. Marking mockable functions and changing the build system a bit is not super-hard. I can offer a patch. If you think it's bad, and we absolutely have to go for function pointers, then I have to investigate this to get an opinion on that.
comment:53 Changed 7 months ago by andrew_b
0001-buildsys-make-it-possible-to-specify-aspell-prefix-f.2.patch
- AC_MSG_NOTICE([aspell support is disabled because gmodule support is not available]) + AC_MSG_ERROR([aspell support is disabled because gmodule support is not available])
AC_MSG_ERROR() breaks the configure. Not sure this change is needed. Probably change NOTICE to WARN instead?
comment:54 in reply to: ↑ 52 Changed 7 months ago by andrew_b
Replying to zaytsev:
I would like to have your opinion on that before I continue working on this.
I don't have a certain opinion. I trust you in this matter.
comment:55 follow-up: ↓ 56 Changed 7 months ago by zaytsev
AC_MSG_ERROR() breaks the configure. Not sure this change is needed.
I noticed that if you do --enable-aspell, but if gmodule is not available, then it's automatically disabled with a warning somewhere in the middle of the long list of checks, so I decided to fix it.
In my opinion, it's fine if the options auto-disable themselves if the prerequisites are not available, if they are not explicitly requested by the user. But if I require as a user --enable-aspell and then it still auto-disables itself without the configure failing, this is a problem. I will not notice it and think that it's enabled, but it's not. So I want the configure to fail, and then I decide if I remove the option or fix the issue.
P.S. I'm still on the fence whether it's better to give a path or a prefix to --enable-aspell - I wonder if you have a strong opinion on that. Giving path means more easily supporting local checkouts. Giving prefix... can use the same value for finding libs to link, eventually?
comment:56 in reply to: ↑ 55 Changed 7 months ago by andrew_b
Replying to zaytsev:
I want the configure to fail, and then I decide if I remove the option or fix the issue.
OK.
P.S. I'm still on the fence whether it's better to give a path or a prefix to --enable-aspell - I wonder if you have a strong opinion on that. Giving path means more easily supporting local checkouts. Giving prefix... can use the same value for finding libs to link, eventually?
We already have both cases:
--with-pcre[=prefix] compile xmlpcre part (via libpcre check) --with-pcre2=DIR root directory path of PCRE2 installation [defaults to /usr/local or /usr if not found in /usr/local]
comment:57 Changed 5 months ago by zaytsev
I have committed the aspell patch as changeset:55e7f8321b23486ceeef7b587bbdf249dfca7532 with prefix, because the ones with dir are called includes, and your examples are actually for prefix. Thanks for the advice! Now one can build with aspell on macOS (and other systems, like Solaris), yay!
comment:58 Changed 5 months ago by zaytsev
Hmmm, now that I can build with Aspell, I get more warnings:
In file included from ../../../src/editor/spell.c:33: /opt/homebrew/include/aspell.h:102:40: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes] struct AspellConfig * new_aspell_config(); ^ void /opt/homebrew/include/aspell.h:195:35: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes] const char * aspell_version_string(); ^ void /opt/homebrew/include/aspell.h:673:49: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes] struct AspellStringList * new_aspell_string_list(); ^ void /opt/homebrew/include/aspell.h:703:47: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes] struct AspellStringMap * new_aspell_string_map(); ^ void ../../../src/editor/spell.c:177:26: warning: 'g_module_build_path' is deprecated [-Wdeprecated-declarations] spell_module_fname = g_module_build_path (NULL, "libaspell"); ^ /opt/homebrew/Cellar/glib/2.80.4/include/glib-2.0/gmodule.h:141:1: note: 'g_module_build_path' has been explicitly marked deprecated here GMODULE_DEPRECATED_IN_2_76 ^ /opt/homebrew/Cellar/glib/2.80.4/include/glib-2.0/gmodule/gmodule-visibility.h:887:36: note: expanded from macro 'GMODULE_DEPRECATED_IN_2_76' #define GMODULE_DEPRECATED_IN_2_76 GMODULE_DEPRECATED ^ /opt/homebrew/Cellar/glib/2.80.4/include/glib-2.0/gmodule/gmodule-visibility.h:30:28: note: expanded from macro 'GMODULE_DEPRECATED' #define GMODULE_DEPRECATED G_DEPRECATED _GMODULE_EXTERN ^ /opt/homebrew/Cellar/glib/2.80.4/include/glib-2.0/glib/gmacros.h:1263:37: note: expanded from macro 'G_DEPRECATED' #define G_DEPRECATED __attribute__((__deprecated__)) ^ 5 warnings generated.
comment:59 Changed 5 months ago by andrew_b
aspell functions should be declared in <aspell.h>.
Call of g_module_build_path() can be removed.
-
src/editor/spell.c
index f316d0fa6..1f9aeba60 100644
a b spell_decode_lang (const char *code) 169 169 static gboolean 170 170 spell_available (void) 171 171 { 172 gchar *spell_module_fname;173 174 172 if (spell_module != NULL) 175 173 return TRUE; 176 174 177 spell_module_fname = g_module_build_path (NULL, "libaspell"); 178 spell_module = g_module_open (spell_module_fname, G_MODULE_BIND_LAZY); 179 g_free (spell_module_fname); 175 spell_module = g_module_open ("libaspell", G_MODULE_BIND_LAZY); 180 176 181 177 if (spell_module != NULL 182 178 && ASPELL_FUNCTION_AVAILABLE (new_aspell_config)
comment:60 Changed 5 months ago by zaytsev
Thank you for the tip! I have created a pull request for Aspell:
https://github.com/GNUAspell/aspell/pull/651
I have committed your suggestion as changeset:0749b6d2d3352805691d1e38e8c1b7daf230a05d - it fixes the problem on our side!
comment:61 Changed 5 months ago by zaytsev
From: Johannes Altmanninger <aclopte@gmail.com> The upcoming fish 3.8 will add a feature flag to officially deprecate "%self," see https://github.com/fish-shell/fish-shell/commit/8d71eef1da7108b37eb50150d2511f576de9fc44 Unfortunately this can cause mc+fish to break for users who configured fish with "set -U fish_features all" which opts into any new feature flag, thus disabling %self expansion. Prevent this potential breakage by using the recommended "$fish_pid", which was introduced in fish 3.0.0 (December 2018). --- src/subshell/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/subshell/common.c b/src/subshell/common.c index dbc32afc2..72f001569 100644 --- a/src/subshell/common.c +++ b/src/subshell/common.c @@ -1176,7 +1176,7 @@ init_subshell_precmd (char *precmd, size_t buff_size) "functions -e fish_right_prompt;" "functions -c fish_prompt fish_prompt_mc; end;" "function fish_prompt;" - "echo \"$PWD\">&%d; fish_prompt_mc; kill -STOP %%self; end\n", + "echo \"$PWD\">&%d; fish_prompt_mc; kill -STOP $fish_pid; end\n", command_buffer_pipe[WRITE], command_buffer_pipe[WRITE], subshell_pipe[WRITE]); break;
comment:62 Changed 5 months ago by zaytsev
I'm wondering, do you need any of the generated Doxygen docs? I've never generated them and don't know if they even work. But even if they do, I'm not sure if they bring any value. If you don't object, I'd actually remove all Doxygen stuff (not the source code comments, of course!)...
comment:63 Changed 5 months ago by zaytsev
Fish patch committed as changeset:c249980ed3265204021a3f5f337b599b8ca3a620 .
comment:64 follow-up: ↓ 65 Changed 5 months ago by zaytsev
RC tarball has been created:
https://midnight-commander.org/nopaste/tarball/mc-4.8.32-pre1.tar.xz
I had to fix the POT-file, because apparently last time it was updated, it was generated in the dirty working copy. Please test. I have noticed that shortcuts have been changed again for Russian translation. Not sure if this is desired.
What about removing Doxygen? I will create a separate ticket if this is OK.
comment:65 in reply to: ↑ 64 Changed 5 months ago by andrew_b
Replying to zaytsev:
I had to fix the POT-file, because apparently last time it was updated, it was generated in the dirty working copy.
What do you mean? I update pot-file and all po-files in the cleanup branch before merge it to the master one.
What about removing Doxygen?
Probably yes.
comment:66 Changed 5 months ago by zaytsev
What do you mean? I update pot-file and all po-files in the cleanup branch before merge it to the master one.
I mean that line numbers do not correspond to the line numbers in the source code, but additionally there are hits from files that aren't even present in the repository like tmp/ydiff.c:2403 - so I assume dirty checkout. Just check the changeset: 2872606832de2d5f5f352601a606d6d8b1ff0e0a to see what I mean.
Changed 5 months ago by zaytsev
- Attachment 0001-x11-fix-Wdeprecated-declarations-for-g_module_build_.patch added
comment:67 Changed 5 months ago by zaytsev
Added fix for x11 deprecation warning.
comment:68 Changed 5 months ago by zaytsev
x11 patch committed to master
comment:69 Changed 4 months ago by zaytsev
- Status changed from new to closed
- Resolution set to fixed
prepare repository for release
download PO-translations from Transifex
store translations in git repo
download the hint translations from Transifex
store translations in git repo
done
create new NEWS wiki page for next version with empty template
add content of current NEWS wiki page to the doc/NEWS file in git repo
new version in Trac
new milestone in Trac
create new tag in git
create tar.(bz2|xz) package files
make checksums for archives
upload source packages and checksums
run ssh
update Wiki start page with latest release number
write an announcement: list user visible changes (bugs and features)
done
create new ticket (type=task, component=adm) for the next release
close ticket for release
close current milestone
done
PRs without corresponding tickets:
up or down in a non-zero column. Regression from 49bc0dd v4.8.31