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

Remove OpenSSL. #322

Merged
merged 2 commits into from
Sep 20, 2019
Merged

Remove OpenSSL. #322

merged 2 commits into from
Sep 20, 2019

Conversation

reitermarkus
Copy link
Member

Closes #229.

@rust-highfive
Copy link

r? @jamesmunns

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@reitermarkus reitermarkus force-pushed the remove-openssl branch 5 times, most recently from cecd61c to f6a8aa0 Compare September 19, 2019 05:41
@reitermarkus
Copy link
Member Author

bors r+

@Emilgardis
Copy link
Member

Shouldnt this get more attention before merging? It's a rather large change

@therealprof
Copy link
Contributor

@Emilgardis I agree. This should have been RFCed so people would know about the reasoning and implications and be able to object.

@Dylan-DPC-zz
Copy link

bors: r-

@bors
Copy link
Contributor

bors bot commented Sep 19, 2019

Canceled

@Dylan-DPC-zz
Copy link

There was a proposal made last year: #229. @Emilgardis @therealprof do you have any issues with this?

@reitermarkus
Copy link
Member Author

The main points are already mentioned in the issue. For me, the most important argument is that this would get rid of a bunch of issues we wouldn't have to deal with anymore. Effectively, I'm the only active maintainer at the moment, so the fewer things to maintain the better. Also, as mentioned in the issue some crates expect OpenSSL not to be installed.

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Sep 19, 2019

Effectively, I'm the only active maintainer at the moment

not true 😉

@reitermarkus
Copy link
Member Author

I meant active in the sense of lines of code. Of course I appreciate you all reviewing those lines. 😉

@Emilgardis
Copy link
Member

I have no issue with this, I think removing OpenSSL is a good idea, but as @therealprof mentioned, making it more formal is imo a good thing, this is essentially a breaking change that could affect users. They should have a chance to voice their opinions. Merging after 1 hour of first approval seems wrong to me. If there's no concerns voiced after x amount of time I think it's fair to say it's safe to merge this however. 👍

@therealprof therealprof added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Sep 19, 2019
@therealprof
Copy link
Contributor

Sorry for only noticing this now:

I meant active in the sense of lines of code. Of course I appreciate you all reviewing those lines. 😉

I think this goes without saying but this is an official Embedded WG tools team project so this is a team effort and there's no "I" in team. Also our rules do apply.

@reitermarkus
Copy link
Member Author

I think this goes without saying but this is an official Embedded WG tools team project so this is a team effort and there's no "I" in team.

You're reading too much into what I am saying. I simply stated a fact, which is that cross isn't a priority for anyone else on the tools team at the moment.

@therealprof
Copy link
Contributor

I simply stated a fact, which is that cross isn't a priority for anyone else on the tools team at the moment.

This may or may not be true but is completely irrelevant. I just wanted to point out that the team is the owner and maintainer of this crate and we do have rules and a code of conduct to adhere to.

@reitermarkus
Copy link
Member Author

reitermarkus commented Sep 20, 2019

completely irrelevant

This is completely relevant. We can't keep OpenSSL support if there isn't anyone to actively maintain it. (In case of an objection that is.)

@therealprof
Copy link
Contributor

@reitermarkus

I'm not objecting to removing OpenSSL just trying to keep focus on the real discussion so an informed decision can be made by the team.

I am objecting to some of the comments which have been made by pointing out that this is a an official project by the Embedded WG and not a one/two man show.

@therealprof
Copy link
Contributor

@adamgreig @ryankurte @Disasm @burrbull

Your opinion and approval is requested here.

@Disasm
Copy link

Disasm commented Sep 20, 2019

I don't understand how cross works, so I can't estimate implications of this PR. As digging into details requires too much effort and time from my side (which I do not have atm), I would prefer to abstain from voting.

@jamesmunns
Copy link
Contributor

jamesmunns commented Sep 20, 2019

The discussion here seems to be going a little beyond just OpenSSL, but I wanted to add a couple points.

First: The cross project was severely neglected before you joined the tools team @reitermarkus. @Dylan-DPC and myself were contributors on the cross project before it was taken over by the Tools team, but I admit I haven't had the bandwidth to help out for a while. I am extremely happy to see Cross being maintained right now, and I want to thank you and Dylan for picking it up over the past weeks.

Second, as this project is owned by the Tools teams, as @therealprof mentions, our policies apply here for the maintenance. The two relevant policies I can see are:

In order to meet the policies above, I would suggest that we:

  1. Either add @Dylan-DPC to the Tools team (through the /wg process), or get an explicit 👍 from the whole Tools team to have him maintain only the cross crate.
  2. @therealprof has already called in the rest of the tools team, so that fits the needs of review by the tools for this breaking change

Additionally if possible, I think we could make this breaking change lower friction if we could document how to correctly add OpenSSL (edit: as a downstream end-user) after this PR has been merged.

@adamgreig
Copy link
Member

I've been staying out of this thread so far since I'm not really involved in cross. Like James I'm pleased to see it getting so much attention! As far as I can tell this is a fairly normal breaking change, the sort of thing that wouldn't typically require an RFC but just a regular approval process. I don't see any issue removing OpenSSL, for all the good reasons already mentioned in the thread. I'm happy to approve the PR.

In terms of team policy I appreciate we should follow our procedures though I don't think any egregious violation has been committed here. I'm very happy for @Dylan-DPC to join the tools team if they want, or to just continue maintaining cross with approval from the tools team. One or the other should happen to keep us in line with our policies.

@therealprof therealprof removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Sep 20, 2019
@therealprof
Copy link
Contributor

... and we have a majority of votes. Thanks for weighing in everybody.

@therealprof
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Sep 20, 2019
322: Remove OpenSSL. r=therealprof a=reitermarkus

Closes #229.

Co-authored-by: Markus Reiter <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 20, 2019

Build succeeded

  • rust-embedded.cross

@bors bors bot merged commit 294dc17 into cross-rs:master Sep 20, 2019
@Dylan-DPC-zz
Copy link

@therealprof thanks for the help. I understand your point here. However, given that the tools team is busy on other projects and life and the staleness that cross has had for the most of this year, it is difficult to wait for the entire team to express their concerns on PRs.

@therealprof
Copy link
Contributor

@Dylan-DPC We're working on building up the team such as bringing in @reitermarkus. In the last few months we've welcomed a few additional members and we're happy to welcome more people interested in helping out.

But it is what it is, this project has been brought into the WG and maintained under our umbrella so everything happening on this project has to happen in compliance with the WG rules, no matter how

difficult to wait for the entire team to express their concerns on PRs

it is. Quite a few people are now relying on cross and also we as WG are committed to delivering quality. This cannot be achieved by skipping a proper review process or the consultation with the rest of the team for breaking changes; however cumbersome or daunting it may be.

@reitermarkus reitermarkus deleted the remove-openssl branch September 24, 2019 22:49
malept added a commit to malept/crypto-hash that referenced this pull request Feb 27, 2020
v0.2 removes openssl from the images.
See: cross-rs/cross#322
malept added a commit to malept/crypto-hash that referenced this pull request Feb 27, 2020
v0.2 removes openssl from the images.
See: cross-rs/cross#322
@jiayihu jiayihu mentioned this pull request Dec 19, 2020
@andrewbanchich andrewbanchich mentioned this pull request Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove OpenSSL
8 participants