Prepare for release mc-4.8.32 #4524
Comments
PRs without corresponding tickets:
up or down in a non-zero column. Regression from [49bc0dd] v4.8.31
|
Replying to zaytsev:
C++ keywords should be moved to cxx.syntax. |
Replying to andrew_b:
Discard this. I was wrong. |
Branch: 4524_cleanup |
Branch: 4524_cleanup |
Maybe can be taken in? |
I think it should be a new ticket. |
|
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... |
Made a PR: autoconf-archive/autoconf-archive#301 |
|
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:
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 :) |
This one is probably tracked here: llvm/llvm-project#20574 |
Another large swath of warnings is due to fallthroughs:
Somehow HAVE_FUNC_ATTRIBUTE_FALLTHROUGH is undefined, so detection is not working correctly. Need to check how to fix that. |
|
Attached patch fixes fallthrough detection for clang. |
|
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? |
Replying to zaytsev:
It depends on type. For standard types (int, void) and types with struct and union keywords
For other types
|
|
Replying to zaytsev:
is in src/vfs/sftpfs/Makefile.am. Is it not enough?
Only use individual constants instead of enum. |
No, it is not enough. Check src/textconf.[ch] - it's also used there. |
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. |
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: |
|
|
Tests fail on macOS :( |
Replying to zaytsev:
m4.include/mc-tests.m4
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. |
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? |
Replying to zaytsev:
Sorry, I haven't tested them yet.
Can't say what is going on. On my linux all tests are passed. |
Do the tests also pass on your machine with
??? |
No, they don't. But they do in case of compile with debugging info |
Replying to zaytsev:
Yes, I am.
It would be great to add some help about --enable-aspell argument in m4-file as well as in doc/INSTALL. |
|
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. |
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. |
AC_MSG_ERROR() breaks the configure. Not sure this change is needed. Probably change NOTICE to WARN instead? |
Replying to zaytsev:
I don't have a certain opinion. I trust you in this matter. |
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? |
Replying to zaytsev:
OK.
We already have both cases: |
I have committed the aspell patch as [55e7f83] 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! |
Hmmm, now that I can build with Aspell, I get more warnings: |
aspell functions should be declared in <aspell.h>.
Call of g_module_build_path() can be removed. index f316d0fa6..1f9aeba60 100644
--- a/src/editor/spell.c
+++ b/src/editor/spell.c
@@ -169,14 +169,10 @@ spell_decode_lang (const char *code)
static gboolean
spell_available (void)
{
- gchar *spell_module_fname;
-
if (spell_module != NULL)
return TRUE;
- spell_module_fname = g_module_build_path (NULL, "libaspell");
- spell_module = g_module_open (spell_module_fname, G_MODULE_BIND_LAZY);
- g_free (spell_module_fname);
+ spell_module = g_module_open ("libaspell", G_MODULE_BIND_LAZY);
if (spell_module != NULL
&& ASPELL_FUNCTION_AVAILABLE (new_aspell_config) |
|
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!)... |
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. |
Replying to zaytsev:
What do you mean? I update pot-file and all po-files in the cleanup branch before merge it to the master one.
Probably yes. |
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: [2872606] to see what I mean. |
|
Added fix for x11 deprecation warning. |
x11 patch committed to master |
done
done
done |
Important
This issue was migrated from Trac:
zaytsev(@zyv)Note
Original attachments:
zaytsev(@zyv) onMay 30, 2024 at 14:27 UTCzaytsev(@zyv) onMay 30, 2024 at 14:59 UTCzaytsev(@zyv) onMay 30, 2024 at 17:27 UTCzaytsev(@zyv) onMay 30, 2024 at 17:50 UTCzaytsev(@zyv) onMay 30, 2024 at 18:53 UTCzaytsev(@zyv) onMay 30, 2024 at 19:10 UTCzaytsev(@zyv) onMay 30, 2024 at 19:14 UTCzaytsev(@zyv) onMay 30, 2024 at 19:21 UTCzaytsev(@zyv) onMay 30, 2024 at 19:28 UTCzaytsev(@zyv) onJun 2, 2024 at 21:25 UTCzaytsev(@zyv) onJun 3, 2024 at 15:52 UTCzaytsev(@zyv) onJun 4, 2024 at 9:33 UTCzaytsev(@zyv) onJun 15, 2024 at 8:54 UTCzaytsev(@zyv) onAug 9, 2024 at 5:41 UTCThe text was updated successfully, but these errors were encountered: