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

Generate and use a config.h header #1088

Merged
merged 6 commits into from
Jul 3, 2023
Merged

Generate and use a config.h header #1088

merged 6 commits into from
Jul 3, 2023

Conversation

eregon
Copy link
Member

@eregon eregon commented Jun 28, 2023

Useful for #1041

Related to #1051

@eregon eregon force-pushed the config-header branch 4 times, most recently from f8ceefd to 552e0ef Compare June 29, 2023 12:43
@eregon eregon marked this pull request as ready for review June 29, 2023 13:15
@eregon eregon requested a review from kddnewton June 29, 2023 13:19
@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

This works with TruffleRuby with oracle/truffleruby#3140, so let's wait that merges and then we can merge this.

Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks good. I actually changed my mind on the src/yarp.c changes though, would you be able to revert those only? Other than that, I think this is good to go. Let me know when the truffleruby PR is merged.

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

I actually changed my mind on the src/yarp.c changes though, would you be able to revert those only?

Do you mean defining YP_VERSION_MAJOR in include/yarp.h instead of in config.h (via autoconf)? What's the rationale?

(BTW this PR should remove those YP_VERSION* from include/yarp.h if we keep it in autoconf, it seems #1051 missed that cleanup)

@kddnewton
Copy link
Collaborator

Specifically I mean using the PACKAGE_VERSION macro. I'm nervous about it conflicting since it's not prefixed with anything.

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Ah sure, happy to change that back to YP_VERSION

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Should I remove the arguments to AC_INIT as well then to not define any PACKAGE_* macros?

@kddnewton
Copy link
Collaborator

Should I remove the arguments to AC_INIT as well then to not define any PACKAGE_* macros?

I think they get defined as empty in that case. But yeah feel free.

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Ah sure, happy to change that back to YP_VERSION

Done.

I think they get defined as empty in that case. But yeah feel free.

Indeed, so I left them in, otherwise they are empty strings.

Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks good

@kddnewton
Copy link
Collaborator

@eregon is this good to go or are we waiting on truffleruby?

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Let's wait the TruffleRuby PR merges to not break the CI here.

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

https://github.com/oracle/truffleruby/actions/runs/5415357250/jobs/9843509793?pr=3140

src/enc/unicode.c: In function ‘utf_8_codepoint’:
src/enc/unicode.c:2233:28: error: conversion to ‘uint32_t’ {aka ‘unsigned int’} from ‘int’ may change the sign of the result [-Werror=sign-conversion]
 2233 |             (0xff >> type) & (byte);
      |                            ^

Not sure why it this error happens there and not in other places. I'll try a trivial fix.

@kddnewton
Copy link
Collaborator

@eregon strange - probably needs to be 0xffu

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Yeah, that goes further, but there are more errors: https://github.com/oracle/truffleruby/actions/runs/5416543973/jobs/9846399623?pr=3140
I'm just using make all-no-debug, so really weird it doesn't happen in YARP's CI.
One difference is that job runs on Ubuntu 20.04 vs 22.04 here.
I'll try to add a 20.04 job here to see if that repros.

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

OK it repros on 20.04: https://github.com/ruby/yarp/actions/runs/5416596186/jobs/9846527321?pr=1088
@kddnewton Could you address these warnings?

@kddnewton
Copy link
Collaborator

@eregon yeah can you rebase off #1103

@eregon
Copy link
Member Author

eregon commented Jun 29, 2023

Thanks!
We'll need a bit longer before merging the TruffleRuby PR, because that needs a small change in mx, and that's currently blocked due to some unrelated CI issue.

@eregon eregon mentioned this pull request Jul 1, 2023
* This was causing problems because mx in TruffleRuby sometimes does `make clean` before `make`.
* The list of generated files was outdated.
* In CRuby the template files are committed,
  and this Makefile is not used, instead common.mk is used.
* It gives different compiler warnings.
@eregon
Copy link
Member Author

eregon commented Jul 3, 2023

The TruffleRuby PR merged, CI is green on rerun, I'll merge

@eregon eregon merged commit 0bd2b72 into main Jul 3, 2023
@eregon eregon deleted the config-header branch July 3, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants