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

AIX support and build tag fixes #62

Merged
merged 3 commits into from
Sep 16, 2024
Merged

Conversation

jtroy
Copy link
Contributor

@jtroy jtroy commented Sep 6, 2024

This PR is to bring #37 up to speed with master. Build tags were needed for the Solaris route info code so it wouldn't be built for GOOS=aix.

Additionally, I ran a go build against each GOOS target and found that Android is broken in master. I made the necessary tweaks to get the build to succeed, but I'm unable to test it. It's in this PR because it was adjacent to the work I was doing, but I can drop it or submit a second PR for it if desired.

Thanks,
John

aklyachkin and others added 3 commits September 6, 2024 11:02

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

hashicorp-cla-app bot commented Sep 6, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

LGTM. While there's no difference between route_info_{aix,solaris}.go, but I think that's fine. Duplicating a small bit of code seems like the right choice here over build tag wrestling.

@schmichael
Copy link
Member

Just need @jtroy to sign the CLA

@jtroy
Copy link
Contributor Author

jtroy commented Sep 9, 2024

LGTM. While there's no difference between route_info_{aix,solaris}.go, but I think that's fine. Duplicating a small bit of code seems like the right choice here over build tag wrestling.

Now that you mention it, I'm happy to combine them into route_info_sysv.go or something along those lines if you prefer to avoid the duplication.

Just need @jtroy to sign the CLA

Signed, thanks!

@sean-
Copy link
Contributor

sean- commented Sep 9, 2024

Because there's no contract that says Solaris and AIX are going to remain compatible, it's fine that they were separate to begin with. Hypothetically, if (when?) these two platforms diverge and there is a bug that requires being addressed, there won't be any shuffling required to move the library bits around if we keep them separate to begin with.

@schmichael schmichael merged commit d05abfa into hashicorp:master Sep 16, 2024
1 check passed
This was referenced Sep 16, 2024
@jtroy
Copy link
Contributor Author

jtroy commented Sep 25, 2024

Thanks @schmichael, @sean-. What's the possibility of getting a release cut? I need to get hashicorp/vault updated as that's the next link in the dependency chain I'm trying to fix.

@schmichael
Copy link
Member

Thanks @schmichael, @sean-. What's the possibility of getting a release cut? I need to get hashicorp/vault updated as that's the next link in the dependency chain I'm trying to fix.

Sure thing; done: https://github.com/hashicorp/go-sockaddr/releases/tag/v1.0.7

@jtroy
Copy link
Contributor Author

jtroy commented Sep 26, 2024

Thanks @schmichael!

@schmichael schmichael mentioned this pull request Dec 4, 2024
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.

None yet

4 participants