Skip to content

New package: c3c-0.7.0#55087

Closed
hyperpastel wants to merge 1 commit intovoid-linux:masterfrom
hyperpastel:c3c
Closed

New package: c3c-0.7.0#55087
hyperpastel wants to merge 1 commit intovoid-linux:masterfrom
hyperpastel:c3c

Conversation

@hyperpastel
Copy link
Copy Markdown

Testing the changes

  • I tested the changes in this PR: YES

New package

Local build testing

  • I built this PR locally for my native architecture, x86_64-libc

@tranzystorekk tranzystorekk added the new-package This PR adds a new package label Apr 16, 2025
@hyperpastel
Copy link
Copy Markdown
Author

I've implemented the changes, thank you so much for your input and feedback 😊

I amended locally and then pushed to the remote, was this the right way of doing this, or should I have done something differently with the 'reviewed changes' and 'conversation' sections GitHub showed?

@meator
Copy link
Copy Markdown
Contributor

meator commented Apr 17, 2025

If I remember the process correctly, you were given a button to apply the recommended changes I gave you. It is best to ignore this button when contributing to void-packages, because it adds one commit per fix. This violates the "commit guidelines" of void-packages.

I amended locally and then pushed to the remote

This is the correct way to deal with feedback in pull requests to void-packages.

@data-man
Copy link
Copy Markdown

Why didn't you choose llvm19?

@hyperpastel
Copy link
Copy Markdown
Author

Ah, I apologise. All the builds of c3c I ever did myself had used llvm17 so I put it here by habit. I will amend once more when I'm home.
I would assume it would work, but should I test locally again? If so it would take a bit because the laptop I have with me is not running Void (yet).
Or should I leave this to the CI?

@data-man
Copy link
Copy Markdown

I would assume it would work, but should I test locally again?
Or should I leave this to the CI?

I don't know, I'm not a Void user. But I am a former C3 user. 😄

@hyperpastel
Copy link
Copy Markdown
Author

hyperpastel commented Apr 20, 2025

Apologies for my absence the past two days. I had some time today again and looked into this, and I'm pretty sure this is a defect in the i686 gcc compiler.

This can be fixed with this relatively simple patch file:

--- a/src/compiler/sema_expr.c
+++ b/src/compiler/sema_expr.c
@@ -5136,7 +5136,7 @@ static inline bool sema_expr_analyse_swizzle(SemaContext *context, Expr *expr, E
 	assert(indexed_type);
 	if (is_lvalue) check = CHECK_VALUE;
 	ASSERT_SPAN(expr, len > 0);
-	int index;
+	int index = swizzle[(int)kw[0]] - 1;
 	bool is_overlapping = false;
 	for (unsigned i = 0; i < len; i++)
 	{
@@ -5145,10 +5145,6 @@ static inline bool sema_expr_analyse_swizzle(SemaContext *context, Expr *expr, E
 		{
 			RETURN_SEMA_ERROR(expr, "The '%c' component is not present in a vector of length %d, did you assume a longer vector?", kw[i], vec_len);
 		}
-		if (i == 0)
-		{
-			index = val;
-		}
 		if ((index ^ val) & 0x10)
 		{
 			RETURN_SEMA_ERROR(expr, "Mixing [xyzw] and [rgba] is not permitted, you will need to select one of them.");

Applying this results in compilation without errors.

Should I include this patch until a better solution is found? (i.e. this gets fixed in a later version of gcc?)

I will also look into using LLVM19, just changing the *depends didn't work and introduced some problems with cmake not finding LLVM anymore

@meator
Copy link
Copy Markdown
Contributor

meator commented Apr 20, 2025

I'm pretty sure this is a defect in the i686 gcc compiler

You're pretty quick to jump to conclusions. The compiler can have bugs, but they shouldn't be the first thing you think of when the compilation process fails.

The problem is caused by upstream using -Werror. Using -Werror in released software is pretty much always wrong. Upstream maintainers cannot anticipate all the compilers and build environments in which their project will be built. What compiled without warnings on their system may not compile without warnings in the future or on a different system. Package maintainers are usually the ones who pay for this ill-advised solution.

The reported warning seems to be harmless. It could be ignored if it didn't cause a compilation error.

Some googling showed me c3lang/c3c#1907

Possible solutions:

  1. Apply the patch you made

    Please make sure that it doesn't change the code's behavior in any way. Bugs introduced by the software distributor are really hard to debug. I wouldn't choose this solution because of this.

  2. Remove -Werror

    This would mean patching/vseding out -Werror in https://github.com/c3lang/c3c/blob/v0.7.0/CMakeLists.txt#L530

    As I said earlier, -Werror usage here is invalid. This will not only solve the i686 build error of 0.7.0, but it could also solve any other similar errors introduced by future updates.

  3. Apply patches from failed to build in gentoo c3lang/c3c#1907

    I would prefer applying patches from this issue rather than your own because the patches and solutions discussed in this issue were acknowledged by an upstream maintainer.

    Some comments in that issue imply that the issue has already been fixed in c3c's master branch and that it will be fixed in the next release. Extracting the commits which solve this issue from master and patching them in here is also a good solution. These patches can be dropped once c3c gets updated. These patches have the highest credibility, because they were created by upstream maintainers and are about to be included in the next release (but they are currently in the master branch which is in development, the upstream maintainers may choose to modify the solution before release).

If you want to be nice, you can also notify upstream of these issues:

  1. Ask them whether they would be willing to reconsider adding -Werror during compilation

    As a package maintainer, I view this as a software bug worth fixing.

  2. Maybe comment on failed to build in gentoo c3lang/c3c#1907 to let upstream maintainers know that several distros (not only Gentoo) are affected by this

@meator
Copy link
Copy Markdown
Contributor

meator commented Apr 20, 2025

I will also look into using LLVM19, just changing the *depends didn't work and introduced some problems with cmake not finding LLVM anymore

The error can be solved with

configure_args="-DLLVM_DIR=$XBPS_CROSS_BASE/lib/llvm/19/lib/cmake/llvm/"

It is strange though. There is not a single mention of llvm19 in the README nor are there any issues. Maybe upstream doesn't support llvm19 as well as it does llvm18 and llvm17?

@hyperpastel
Copy link
Copy Markdown
Author

Thank you for your fast and detailed response yet again, meator 🥰

I apologize if my judgement is too quick. It just struck me as something weird that the same few lines of code compile without a warning on all other platforms, and only on i686, a warning is generated (which, unless I'm reading something very wrong, should not be).

I have brought this to the attention of the maintainer, and I will also mention the -Werror flag.

As for the solutions, I then agree that removing this flag is probably the best.

I've read the issue you've linked, but I believe this addresses different files, namely src/utils/file_utils.c, src/compiler/sema_const.c and src/compiler/sema_stmts.c, whereas our culprit resides in src/compiler/sema_expr.c and is still present in both the dev and master branch 🫠

I will wait a bit for a direct response from maintainers, if nothing emerges from that, I would probably go down the vsed route 😊

@hyperpastel
Copy link
Copy Markdown
Author

As for the llvm19 situation, there is indeed no mention, and this was also the cause why I always built it with llvm17 and then also habitually used llvm17 in the template.

I did ask if there was downsides to using llvm19 over llvm17 when building from source, which I was told there aren't. Probably this is more about the actual software behavior though rather than the CMake behavior 😛

I could always try to test the software's behavior if linked with llvm19, but should we include configure_args like that in the template? Or would it not better to wait until this is perhaps more popular, mentioned in upstream and has a cleaner solution? 🤔

@meator
Copy link
Copy Markdown
Contributor

meator commented Apr 20, 2025

I've read the issue you've linked, but I believe this addresses different files, namely src/utils/file_utils.c, src/compiler/sema_const.c and src/compiler/sema_stmts.c, whereas our culprit resides in src/compiler/sema_expr.c and is still present in both the dev and master branch

Ah, didn't notice that. This showcases that such issues are common in c3c and that removing -Werror might be beneficial.

I did ask if there was downsides to using llvm19 over llvm17 when building from source, which I was told there aren't. Probably this is more about the actual software behavior though rather than the CMake behavior

If you've got confirmation that no issues will arise from using llvm19, you should use it.

@hyperpastel
Copy link
Copy Markdown
Author

hyperpastel commented Apr 20, 2025

I would prefer applying patches from this issue rather than your own because the patches and solutions discussed in this issue were acknowledged by an upstream maintainer.

I discussed this with the upstream and this PR ended getting merged, so I would patch accordingly with the confirmation of the maintainer. They would like to keep -Werror.

I've spent a bit more time looking into using llvm19. After setting -DLLVM_DIR like you suggested, I was able to build with it, except for my now favorite architecture i686 🫠

I failed with the following error:

ninja: error: '/usr/lib/libz3.so', needed by 'c3c', missing and no known rule to make it

I don't know if the following is possible, but - I believe that setting LLVM_DIR like this messed with the inner CMake which provides LLVM, which caused it to not properly find libz3.so on the i686 architecture.

So I thought to add both the path you provided earlier (which I was also able to shorten a bit, CMake still finds it without the last parts!) and the "normal" $XBPS_CROSS_BASE/usr/lib/ path, so that LLVM may properly find libz3.so.

However, I was unable to set -DLLVM_DIR with both these values in configure_args, so I set it in -DCMAKE_PREFIX_PATH instead and it works with this!

I also took the liberty to add a variable for the version of the llvm-packages that c3c depends on 😊

@hyperpastel
Copy link
Copy Markdown
Author

c3lang/c3c#2103 has been merged, so in the next release both the extra configure_args and the patch will no longer be needed 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-package This PR adds a new package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants