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

Fixes issue #49 #106

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Fixes issue #49 #106

merged 3 commits into from
Mar 21, 2022

Conversation

alecgarza96
Copy link
Contributor

Fixes issue #49

@alecgarza96 alecgarza96 changed the title added .s file extension for assembly support Fixes issue #49 Mar 10, 2022
@alecgarza96
Copy link
Contributor Author

added .s file extension for assembly support

@polac24
Copy link
Collaborator

polac24 commented Mar 19, 2022

Are you sure this fixes #49? The original problem was reported for CNIOBoringSSL library, which uses *.S (not *.s) files (e.g. https://github.com/apple/swift-nio-ssl/blob/main/Sources/CNIOBoringSSL/crypto/fipsmodule/aesni-gcm-x86_64.linux.x86_64.S).
isSuffixed is case sensitive so it wouldn't consider aesni-gcm-x86_64.linux.x86_64.S as a valid input file.

@alecgarza96
Copy link
Contributor Author

I can make the change, would it be best to support both *.S and *.s? Or does CNIOBoringSSL not support *.s at all?

@polac24
Copy link
Collaborator

polac24 commented Mar 21, 2022

I think having two or with .s and .S is OK. No idea why CNIOBoringSSL uses .S instead of LLVM-documented .s.

@alecgarza96
Copy link
Contributor Author

I committed the changes to this PR and they appear to be reflected when I verify in the Files Changed section. Let me know if this works, or if a new PR should be opened. New to this repo

@alecgarza96
Copy link
Contributor Author

image
getting following error as reason for failed tests, troubleshooting

@polac24
Copy link
Collaborator

polac24 commented Mar 21, 2022

That seem to be a reason of a non-stable test setup. I tried to stabilize it in #81, but still without success.

@polac24 polac24 merged commit 8241914 into spotify:master Mar 21, 2022
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