Ticket #4524 (closed task: fixed)

Opened 11 months ago

Last modified 4 months ago

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


Change History

comment:1 follow-up: ↓ 2 Changed 11 months ago by zaytsev

PRs without corresponding tickets:

In release 4.8.31, mcedit loses the column position when navigating

up or down in a non-zero column. Regression from 49bc0dd v4.8.31

Keywords taken from: https://en.cppreference.com/w/c/keyword https://en.cppreference.com/w/cpp/keyword

comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 11 months ago by andrew_b

Replying to zaytsev:

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:3 in reply to: ↑ 2 Changed 11 months ago by andrew_b

Replying to andrew_b:

C++ keywords should be moved to cxx.syntax.

Discard this. I was wrong.

comment:4 Changed 11 months ago by andrew_b

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

comment:7 Changed 8 months ago by zaytsev

Yet another PR without ticket:

https://github.com/MidnightCommander/mc/pull/196

Asked for tests.

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.

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

comment:9 Changed 8 months ago by zaytsev

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

Replying to andrew_b:

I think it should be a new ticket.

#4541

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: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.

comment:17 Changed 7 months ago by zaytsev

Need to check how to fix that.

Attached patch fixes fallthrough detection for clang.

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)

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

Changed 7 months ago by zaytsev

Changed 7 months ago by zaytsev

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

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?

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

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.

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

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.

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) 
    169169static gboolean 
    170170spell_available (void) 
    171171{ 
    172     gchar *spell_module_fname; 
    173  
    174172    if (spell_module != NULL) 
    175173        return TRUE; 
    176174 
    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); 
    180176 
    181177    if (spell_module != NULL 
    182178        && ASPELL_FUNCTION_AVAILABLE (new_aspell_config) 
Last edited 5 months ago by andrew_b (previous) (diff)

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

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.

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

NEWS-4.8.33

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

#4572

close ticket for release
close current milestone

done

Note: See TracTickets for help on using tickets.