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

--manifest-path path/to/NonCargo.toml fails #6690

Open
gnzlbg opened this issue Feb 21, 2019 · 7 comments
Open

--manifest-path path/to/NonCargo.toml fails #6690

gnzlbg opened this issue Feb 21, 2019 · 7 comments
Labels
A-cli Area: Command-line interface, option parsing, etc. C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed. S-propose-close Status: A team member has nominated this for closing, pending further input from the team

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 21, 2019

cargo build --manifest-path=NotCargo.toml 

fails with

the manifest-path must be a path to a Cargo.toml file

but my NotCargo.toml is a manifest file.

So either the option needs to be called --cargo-toml-path, or it should IMO accept any TOML file that's a valid manifest path.

Motivation: working around cargo bugs often requires having many manifests files and using different ones depending on what one is trying to do (e.g. build for two different editions, build without dev-dependencies in the dependency graph, etc.).

@gnzlbg gnzlbg added the C-bug Category: bug label Feb 21, 2019
@mssun
Copy link

mssun commented May 30, 2019

Yes, I met the same issue. Can we just delete this check and update related testcases?

if !path.ends_with("Cargo.toml") {
failure::bail!("the manifest-path must be a path to a Cargo.toml file")
}

@AmitPr
Copy link

AmitPr commented Nov 17, 2021

This seems like a valuable feature to have, in the case where one codebase has to run through several unique compilation chains, to support things like having a separate Cargo.toml for each target. Looking at this and #10083

@epage epage added the S-triage Status: This issue is waiting on initial triage. label Oct 31, 2023
@epage
Copy link
Contributor

epage commented Oct 31, 2023

Going to write up some quick thoughts from different angles.

This would only be able to work with --manifest-path; we wouldn't be able to auto-discover these files (likely people realize this but wanted to be explicit).

Since this was opened, we took advantage of this flexibility in adding support for single-file packages in #12289 and then allowing Cargo.toml to be treated similarly in #12281. We'd need to be careful in terms of what doors generalizing this might close to us in the future.. Maybe if we just loosened the restriction to .toml? Things might be a little weird with our stance in #45 though maybe with the lack of auto-discovery, this wouldn't be a problem.

Depending on what all was different between files, this could also cause a lot of churn in Cargo.lock.

The alternative Cargo.toml files wouldn't be accessible in published packages.

For some of the use cases, I wonder if features like the following would be better

Or just defining separate packages.

@epage epage added S-needs-team-input Status: Needs input from team on whether/how to proceed. S-propose-close Status: A team member has nominated this for closing, pending further input from the team and removed S-triage Status: This issue is waiting on initial triage. labels Oct 31, 2023
@epage
Copy link
Contributor

epage commented Oct 31, 2023

Based on the above plus our design principle of having opinionated workflows,. I'm going to propose to the cargo team that we close this.

@ChrisDenton
Copy link
Member

I don't feel strongly about this at all but it seem does seem unfortunate that --manifest-path is the only flag where an exact file name is required? Obviously there are a lot of caveats and reasons why using anything but Cargo.toml is bad in the general case but like, what's wrong with wanting to test with Cargo.toml.orig, etc?

Or maybe it should have always been --manifest-directory and not make you type out the redundant Cargo.toml (can't change that now, obviously).

@epage
Copy link
Contributor

epage commented Nov 4, 2023

Part of this comes down to use cases. For most I've seen, its to cover for missing features of Cargo and we'd like want to focus on those. If there are more, let us know!

For some context, this isn't just a superficial change in the CLI and we're done.

As of right now, we track path sources by directory, preventing us from being able to convert from that back to a non-canonical Cargo.toml. We already need to partially address this for #12207 as something kind of like our Source ID ends up in the user-facing Package Id Spec. In working on that, it'll help us have an idea of how well we can also support pointing directly to a file and not just the directory more generally.

That involves the infrastructure side. The next part is if we should allow having dependencies to a Cargo.toml.something. We'd need to make sure the above does not accidentally affect this. For #12207, we've deferred that out. If we wanted it, we'd likely want to wait for that so we can develop it in lockstep. Supporting this for dependencies seems backwards. Not supporting this for dependencies seems inconsistent.

We'd also need to consider how we tell the formats apart. Right now,, the rules are

  • No extension or .rs: cargo script
  • Cargo.toml: manifest

I'd really not like to get into sniffing the file contents and guessing as a small bug could have very strange results. We'd need to define what we use to recognize something is a regular manifest format.

@ChrisDenton
Copy link
Member

Fair enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed. S-propose-close Status: A team member has nominated this for closing, pending further input from the team
Projects
None yet
Development

No branches or pull requests

6 participants