Skip to content

Conversation

@namse
Copy link
Contributor

@namse namse commented Sep 10, 2024

I am doing a Windows MSVC cross-build in a Linux environment. Although I clearly have the intsafe.h file in my environment, I encountered an error saying "Intsafe.h not found." Upon checking, it turned out to be a case sensitivity issue. It seems that building on a Windows host doesn't have this issue because it's case-insensitive.

Therefore, this PR corrects the name from Intsafe.h to intsafe.h. It looks like there was a typo in the case, as the Win SDK also lists it as intsafe.h.

Reference: https://learn.microsoft.com/en-us/windows/win32/api/intsafe/

@petterreinholdtsen
Copy link

I agree that this is a typo. I am unsure how the source is maintained. Will merging the pull request here on github cause problems with syncing with gitlab.xiph.org?

@ePirat
Copy link

ePirat commented Sep 10, 2024

@petterreinholdtsen It should be merged on the Xiph GitLab

@namse
Copy link
Contributor Author

namse commented Sep 10, 2024

Then, should I make a merge request on gitlab side?

@ePirat
Copy link

ePirat commented Sep 10, 2024

Either that or maybe some of the maintainers here can just push it directly on GitLab. (I can also do it if I get the OK from an Opus maintainer)

Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

lgtm fwiw

@namse
Copy link
Contributor Author

namse commented Sep 10, 2024

@rillian
Could you please let me know what needs to be done for this PR to be merged into the opus codebase?

Since this is a blocking issue for MSVC cross-builds on case-sensitive OSs, I hope it can be released as soon as possible.

Of course, there may not be many people who need this changes like I do, but it is currently a significant roadblock in our product development.

@petterreinholdtsen
Copy link

petterreinholdtsen commented Sep 10, 2024 via email

@ePirat
Copy link

ePirat commented Sep 10, 2024

Well @rillian for example

@tmatth
Copy link
Member

tmatth commented Sep 10, 2024

@namse I created an MR from this here, we can merge when it passes CI:

https://gitlab.xiph.org/xiph/opus/-/merge_requests/123

Your authorship is preserved.

@tmatth
Copy link
Member

tmatth commented Sep 10, 2024

Hrm looks like our CI no longer allows for noreply.github.com email addresses (although previously in the git log we see e.g. 009d741)

@namse can you amend your commit with a valid email address?

For reference, here's the full error:

ERROR: Commit message check failed
  commit: 34564b0f6c7c1ddbc5aced0a6d87a5224edab80c
  author: Nam Se Hyun <namse@users.noreply.github.com>
  Rename `Intsafe.h` to `intsafe.h` for case-sensitive OS
After correcting the issues below, force-push to the same branch.
This will re-trigger the CI.
---
1. git author email invalid
Please set your name and email with the commands
    git config --global user.name Your Name
    git config --global user.email your.email@provider.com

@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

@tmatth Sure, I changed email and pushed again.

commit 7aedb2c4edf223a37456167876d216caf3655002 (HEAD -> patch-1, origin/patch-1)
Author: Nam Se Hyun <skatpgusskat@naver.com>
Date:   Tue Sep 10 15:42:46 2024 +0900

    Rename `Intsafe.h` to `intsafe.h` for case-sensitive OS

@tmatth
Copy link
Member

tmatth commented Sep 11, 2024

Merged in 1884ec0, thanks.

@tmatth tmatth closed this Sep 11, 2024
@namse namse deleted the patch-1 branch September 11, 2024 12:56
@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

@tmatth Would it be possible to know when this change is expected to be released? It's currently a blocking issue for our product, so we would greatly appreciate it if it could be released at your earliest convenience.

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.

5 participants