Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add semicolon to the buffer atime assignment #2661

Merged
merged 2 commits into from
May 27, 2022

Conversation

Strech
Copy link

@Strech Strech commented May 22, 2022

I just accidentally found a forgotten ; while compiling it on M1

@chrisseaton
Copy link
Collaborator

We'd have already fixed this if we merged my PR!

#2633

@Strech
Copy link
Author

Strech commented May 23, 2022

@chrisseaton Oh, thanks for pointing out. Should I keep this PR open or ...?

@chrisseaton
Copy link
Collaborator

Sore that comment was for the reviewers of this PR, rather than directed at you.

@Strech
Copy link
Author

Strech commented May 23, 2022

We can co-authored it 😄 I think it might be tough to push a big change, but this mistype is clearly the small fix we can just merge without much hassle ✌🏼

@chrisseaton
Copy link
Collaborator

It's great for you to author this PR and get it merged - I'm just saying to the wider team that this was already fixed if we'd merged it rather than deferring the work.

@eregon
Copy link
Member

eregon commented May 25, 2022

This sounds like an incorrect lib/cext/include/truffleruby/config_darwin_aarch64.h header used.
I'd think darwin-aarch64 should have at least one of these 3 HAVE_STRUCT_STAT_ST_ATIM* defines set.

How did you create your lib/cext/include/truffleruby/config_darwin_aarch64.h, or did you modify config.h?
It should have errored earlier if missing:

#error Unsupported platform

This is why I want proper darwin-aarch64 support and not partial patches, otherwise people build it and find it's half broken.
I want something tested in CI and correct, that's why I closed #2633.
It's nearly ready, see #2657

@eregon
Copy link
Member

eregon commented May 25, 2022

We can of course add the missing ;, but it seems it's really just the side effect of something more fundamentally broken/hacked to hit that path (maybe it should be an #error instead).

@Strech
Copy link
Author

Strech commented May 25, 2022

How did you create your lib/cext/include/truffleruby/config_darwin_aarch64.h, or did you modify config.h?
It should have errored earlier if missing:

It was giving me Unsupported platform error only after I've manually fixed a semicolon issue

@eregon
Copy link
Member

eregon commented May 26, 2022

I tried to repro it locally with

diff --git a/lib/cext/include/ruby/config.h b/lib/cext/include/ruby/config.h
index 9f4c3bd5b5..a0b7fd2083 100644
--- a/lib/cext/include/ruby/config.h
+++ b/lib/cext/include/ruby/config.h
@@ -9,7 +9,7 @@
 
 #else
 
-#if defined(__x86_64__)
+#if defined(__x86_64__) && 0
 #include <truffleruby/config_linux_amd64.h>
 #elif defined(__aarch64__)
 #include <truffleruby/config_linux_aarch64.h>

That gives:

Building org.truffleruby.cext with GNU Make... [rebuild needed by GNU Make]
make[1]: Entering directory '/home/eregon/code/truffleruby-ws/truffleruby/src/main/c/truffleposix'
In file included from truffleposix.c:44:
/home/eregon/code/truffleruby-ws/truffleruby/lib/cext/include/ruby/config.h:17:2: error: Unsupported platform
#error Unsupported platform
 ^
truffleposix.c:470:25: error: expected ';' after expression
  buffer->atime_nsec = 0
                        ^
                        ;
2 errors generated.

So clang (from the toolchain) shows both errors at the same time, and so keeps compiling even though a header errored.
I guess there is nothing we can do about that, except maybe knowing it's always worth fixing the first error before looking at the other errors.

Now that I understand how this error is seen, I'll merge this and a couple more cleanups to truffleposix.c.

@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label May 26, 2022
@eregon eregon force-pushed the fix-truffleposix-mistype branch from bb0c828 to 012ae87 Compare May 27, 2022 09:43
@graalvmbot graalvmbot merged commit c52e632 into oracle:master May 27, 2022
@eregon eregon added this to the 22.2.0 milestone May 27, 2022
@Strech Strech deleted the fix-truffleposix-mistype branch May 30, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants