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

Implemented binary_deps behind a feature #407

Closed
wants to merge 1 commit into from
Closed

Implemented binary_deps behind a feature #407

wants to merge 1 commit into from

Conversation

UebelAndre
Copy link
Collaborator

The binary_deps flag brings in some pretty large dependencies. This PR creates the feature binary_deps and wraps all uses of that functionality behind it.

Additionally, the feature cli was added to have additional control over the dependencies required by cargo-raze when used as a library.

@UebelAndre
Copy link
Collaborator Author

The version of rust in github actions was updated to solve for

 error[E0658]: `match` is not allowed in a `const fn`
Error:    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/socket2-0.4.0/src/lib.rs:156:9
    |
156 | /         match address {
157 | |             SocketAddr::V4(_) => Domain::IPV4,
158 | |             SocketAddr::V6(_) => Domain::IPV6,
159 | |         }
    | |_________^
    |
    = note: see issue #49146 <https://github.com/rust-lang/rust/issues/49146> for more information

@UebelAndre
Copy link
Collaborator Author

@acmcarther Can I get you to take a look at this and some other PRs?

illicitonion pushed a commit to bazelbuild/rules_rust that referenced this pull request Mar 30, 2021
This PR updates the resolver to use a version of `cargo-raze` which does not require any dependencies from the `binary_deps` feature. This should make it much easier and faster to compile.

The upstream PR is google/cargo-raze#407
@UebelAndre
Copy link
Collaborator Author

@acmcarther Hey, just pinging this one again 🙏

@UebelAndre
Copy link
Collaborator Author

@acmcarther Can you please take a look at this 🙏

@dfreese
Copy link
Collaborator

dfreese commented Apr 7, 2021

I'm quite hesitant about feature flags as it makes testing all combinations more difficult. The cli and binary-deps features here seem to bifurcate the codebase.

Is the intention is to use cargo-raze as a library, rather than a binary?

@UebelAndre
Copy link
Collaborator Author

Yes, the intent is to use cargo-raze as a library

@illicitonion
Copy link
Collaborator

Wider context here is that we're using cargo-raze as a library for the crate_universe rule in rules_rust, and it's much easier for us to bootstrap binaries for rules_rust without needing to pull in and build openssl-sys, libgit2-sys, and libssh2-sys, which are only needed for binary_deps.

@dfreese
Copy link
Collaborator

dfreese commented Apr 7, 2021

Got it. So the typical approach here would be to pull things out into a -core or -base crate. As the PR stands, a large chunk of the planning code ends up functioning in two different ways, which is really hard to maintain going forward.

Is there was a way for this feature break to happen strictly at a module level? That will probably help with the discussion here.

@UebelAndre
Copy link
Collaborator Author

How does a large chunk of the planning code change?

@google-cla
Copy link

google-cla bot commented Apr 7, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@UebelAndre
Copy link
Collaborator Author

@illicitonion I'll need you to address CLA 🙏

@illicitonion
Copy link
Collaborator

@googlebot I consent.

yagehu pushed a commit to yagehu/rules_rust that referenced this pull request Apr 23, 2021
This PR updates the resolver to use a version of `cargo-raze` which does not require any dependencies from the `binary_deps` feature. This should make it much easier and faster to compile.

The upstream PR is google/cargo-raze#407
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented May 18, 2021

@dfreese re bazelbuild/rules_rust#415 (comment)

However, there wasn't any further discussion specifically on the module question.

#407 (comment) is me trying to understand what you're saying so I can appropriately assess your module question

@dfreese
Copy link
Collaborator

dfreese commented May 31, 2021

@UebelAndre that wasn't intended to be the main point of that comment, the main thing is that planning.rs and metadata.rs now have two paths #[cfg(feature = "binary_deps")] and #[cfg(not(feature = "binary_deps"))] for quite a number of things. This splits tests and makes it harder to keep track of what any specific code does.

If there could be one spot where we could do:

#[cfg(feature = "binary_deps")]
mod some_name_for_raze_binary_related_functionality
#[cfg(not(feature = "binary_deps"))]
mod fake_some_name_for_raze_binary_related_functionality as some_name_for_raze_binary_related_functionality;

and the rest of the code could remain free of feature gates, that would convince me that this is maintainable within cargo-raze, or that there's a clear set of functionality that can be pulled into a -core or -base crate.

edit: formatting

@UebelAndre UebelAndre closed this Aug 25, 2021
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Aug 28, 2021

@dfreese Can you provide a concrete example of this? Specifically, a crate where that was done to test branching behavior in a couple different places within a project?

@UebelAndre
Copy link
Collaborator Author

Also, I feel so strongly that this feature is just an anti-pattern and should be deleted (#305). It's undefined behavior and something that could probably be solved without requiring this tool to have a built-in solution. It should just be removed.

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