Skip to content

Conversation

jMichaelA
Copy link

Extends the ICONV_CONST const preprocessor for iconv to illumos based distributions.

@devnexen
Copy link
Member

Yes I looked at it last night, was (pleasantly) surprised that __illumos__ exists ! Will relook a bit later today. Cheers.

@jMichaelA
Copy link
Author

Thanks devnexen. I made this for OmniOS, and I ran through a basic installation. Let me know if you have any questions that I can help with!

@devnexen
Copy link
Member

The only question I would have .. should solaris have the same treatment ? solaris is still supported until 2027 I believe.

@jMichaelA
Copy link
Author

jMichaelA commented Jun 25, 2025

defined(__sun) && defined(__SVR4) works for OmniOS and should work to support more Solaris distros as well. So that would be a good option as well.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

lgtm

@devnexen devnexen requested a review from a team September 16, 2025 20:52
@Girgias
Copy link
Member

Girgias commented Sep 21, 2025

@devnexen are you planning on merging this?

@devnexen
Copy link
Member

well I asked RM opinion on this otherwise I ll wait post 8.5

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

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

well I asked RM opinion on this otherwise I ll wait post 8.5

Sorry for the delay, RM approval, technical review not performed

@petk
Copy link
Member

petk commented Oct 15, 2025

Here, there is also this PR opened #16847

I think adjusting the preprocessor check for all platforms is quite difficult to do correctly. For example, on newer NetBSD versions, const isn't needed anymore #20089

So, I think that configuration phase check should resolve this issue.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants