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

Add vendored openssl feature #66

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Conversation

BlackHoleFox
Copy link
Contributor

OpenSSL is kind of a pain to deal with on macOS, so this tries to make using cargo-instruments more seamless by letting users compile a vendored version of the source instead. This is especially true if you don't use Homebrew, which makes getting the openssl-dev headers much more work.

@cmyr
Copy link
Owner

cmyr commented Jul 4, 2022

Thanks for the PR. I don't fully understand how this works, so maybe you can help me out?

since ssl is only used through cargo, I would expect cargo to provide an appropriate feature to enable vendoring. Is this not the case? An issue I worry about here is that we now need to make sure we keep our version of ssl up to date with what is being used by cargo; maybe not a huge annoyance, but not nothing?

I guess it seems strange to me for us to bring in a crate that we don't use directly, and set features on it, with the intention of changing what is used by a different crate? Can we guarantee that the feature resolver will always handle this correctly?

I also don't think this should be a default feature, unless there's a compelling argument that I'm missing.

@Crosse
Copy link

Crosse commented Jul 21, 2022

One other reason to bring this in is because it enables those who use MacPorts or pkgsrc (👋) to successfully build this at all. (I just went through this anguish and came up with mostly the same diff.) Without it, there are arcane errors about missing _iconv* symbols coming from libgit2. (It's only after an hour of googling does the connection between the libgit2 errors and openssl become—I won't say apparent, but visible.)

I agree that I don't think it should be a default feature, but having it as an optional feature would be great. I was quite literally coming here to propose that before I found this PR.

Also, thanks for such a wonderful command!

@BlackHoleFox
Copy link
Contributor Author

Missed the original comment on this one, sorry. I took another look and realized that Cargo actually has its own vendored-openssl feature so I just used that instead of using a direct openssl dependency here. Also made it the non-default since its 2/1 there and I'm good with it not the default.

@cmyr This is ready for review again, and thanks for the nudge to look at cargo itself first.

Copy link
Owner

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Perfect this looks right to me, thanks. I'll cut a release shortly!

@cmyr cmyr merged commit e5027b1 into cmyr:master Jul 26, 2022
@BlackHoleFox BlackHoleFox deleted the vendored-openssl branch July 26, 2022 15:28
@cmyr
Copy link
Owner

cmyr commented Jul 27, 2022

@BlackHoleFox @Crosse published as 0.4.7.

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

3 participants