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

Integer conversion on Solaris and Illumos #278

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Integer conversion on Solaris and Illumos #278

merged 1 commit into from
Sep 24, 2020

Conversation

francescocarzaniga
Copy link
Contributor

The library was not used anywhere and it prevented the build from running on Illumos, this fixes it as far as I can tell.

Fix illumos support.
@jhpratt
Copy link
Member

jhpratt commented Sep 22, 2020

Duplicate of #277, which is awaiting a response from the author.

@jhpratt jhpratt closed this Sep 22, 2020
@jhpratt jhpratt added A-targets Area: support for various targets C-duplicate Category: exact duplicate labels Sep 22, 2020
@francescocarzaniga
Copy link
Contributor Author

Oops, this is embarrassing.

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

No worries! If you'd like to check and make the appropriate changes I noted on the other PR, I'll happily merge this one.

@francescocarzaniga
Copy link
Contributor Author

I made the required changes to my branch, but this PR is closed. Want me to open a new one?

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

I'm on mobile now, but I'll look at it later today if you push the changes up.

@francescocarzaniga
Copy link
Contributor Author

They have been pushed, but they don't show up on a closed PR afaik.

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

Quickly looking into things: it looks like I'm unable to reopen because I've force pushed to main since you forked (I know, bad habit). It looks like there's a workaround, as your commits surely don't cause any conflict.

Thank you! Sorry about the hassle.

@francescocarzaniga
Copy link
Contributor Author

I've also force pushed to my PR branch. Let me know if you need me to revert before reopening.

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

Looks like my force-pushing isn't actually the issue here, as your branch includes ccd997e, the latest commit on this repo's main branch.

We've got a few options here:

  1. You can create a new PR
  2. I can cherry-pick the commit from your branch, which would still give you credit in the commit history.
  3. Somehow figure out how to reopen this PR. I presume you'd have to dig through reflogs to find the original commit hash and force-push that.

I don't have a preference, so it's up to you.

@francescocarzaniga
Copy link
Contributor Author

You should be able to reopen this PR now. If that's not the case, just go ahead with option 2.

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

Unfortunately I'm not able to for some reason. I'll reach out to GitHub to try and figure out why, since I see absolutely no reason for that to be the case now. If you push up the changes — again — I'll cherry-pick it.

@francescocarzaniga
Copy link
Contributor Author

Got it. I've pushed the changes again.

@jhpratt jhpratt merged commit bb39ef9 into time-rs:main Sep 24, 2020
@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2020

Meh, managed to use the official GitHub CLI to merge it 😆

Whatever works I suppose.

@jhpratt jhpratt removed the C-duplicate Category: exact duplicate label Sep 24, 2020
@jhpratt jhpratt mentioned this pull request Sep 24, 2020
jhpratt pushed a commit that referenced this pull request Sep 24, 2020
jhpratt pushed a commit that referenced this pull request Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-targets Area: support for various targets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants