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

Set an environment variable for tests to find executables. #7697

Open
wants to merge 1 commit into
base: master
from

Conversation

@ehuss
Copy link
Contributor

ehuss commented Dec 11, 2019

This adds the environment variable CARGO_BIN_EXE_<name> so that integration tests can find binaries to execute, instead of doing things like inspecting env::current_exe().

The use of uppercase is primarily motivated by Windows whose Rust implementation behaves in a strange way. It always ascii-upper-cases keys to implement case-insensitive matching (which loses the original case). Seems less likely to result in confusion?

Closes #5758.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Dec 11, 2019

r? @Eh2406

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

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Dec 11, 2019

I like it. But I think the team should discuss before we merge.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 11, 2019

@rfcbot fcp merge

Looks like a great feature to me!

@rfcbot

This comment has been minimized.

Copy link
Collaborator

rfcbot commented Dec 11, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Dec 11, 2019

TIL that HFS normalizes unicode differently from APFS.

Copy link
Member

joshtriplett left a comment

This commit seems to be doing many different things. It's adding documentation for other methods, changing some existing documentation files (including things like changing dashes to Unicode dashes), and adding the new variable/test/documentation. Could you please split the commit, and make one logical change per commit? (I would also request that the other commits not be in one omnibus pull request with several unrelated changes.)

Apart from that: I'm quite uncomfortable with the "convert to uppercase" approach. I appreciate that you've taken the care to do so in what seems like a correct manner for Unicode, but it still doesn't seem like a good idea to me. I would prefer to always match the exact name used in the Cargo.toml file (or the exact name that Cargo infers the binary name from if not explicitly specified), without performing any kind of conversion.

That aside, I do like the concept, and I think it will make tests simpler and more robust.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 14, 2020

@rfcbot concern should-use-same-case-as-Cargo.toml

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 15, 2020

I'll split the unrelated things out.

As for the uppercase thing, I think it will be extremely difficult to support Windows without it. For example, the Rust standard library loses case (here). In general, supporting case-sensitive environment variables can be difficult (and also unusual in my experience). Are there specific concerns about what could happen? I'm well aware that that there can be difficult edge cases, but I think for the vast majority of cases it should be fine.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 15, 2020

I appreciate that Windows is not case-sensitive. Unix and Linux, however, are. If there's no better solution available for Windows than uppercasing the variable name, then that should only occur on Windows.

Files differing only by case do indeed appear in real projects and real use cases. Furthermore, on a case-sensitive platform, changing the case of a binary name refers to a different binary, and users of such platforms (myself included) will find that behavior surprising, unexpected, and unwelcome.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 15, 2020

Also, different environments uppercase Unicode differently, and some Microsoft tools and libraries will handle the example you have (ß) differently.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 15, 2020

I disagree that we should be doing something different only on Windows here, that's generally not what Cargo does and can cause compatibility issues when your project works on Linux and doesn't work on Windows. In general I believe we try to have uniform behavior across all platforms, which in cases like this involves taking the intersection of possible behaivors and making that the API that we expose.

I don't think that we really need to worry all that much about unicode here. While it's of course a minefield if you're constantly knee-deep in unicode we're talking about Cargo projects and binary names, which 99.9% of the time don't even contain anything beyond ASCII. I don't think that an extremely small slice of projects which may have odd upper-casings in unicode should cause us to force everyone to deal with Windows-specific behavior.

@ehuss ehuss force-pushed the ehuss:bin-test-env branch from 9c56a4b to 07dce15 Jan 15, 2020
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 15, 2020

@alexcrichton I don't want everyone to deal with Windows-specific behavior either. Uppercasing on all platforms makes everyone have to deal with case-insensitivity, which is Windows-specific behavior. On a Linux platform, I expect names to be case sensitive, and squelching case is a bug.

Programs distinguished only by case isn't a theoretical issue, it's quite common. As one example, many Linux systems have both /usr/bin/HEAD and /usr/bin/head, which are two completely different programs. (The former sends an HTTP HEAD request, the latter outputs the first lines of a file.)

What I'm suggesting is this: use the actual case on all platforms, have test programs reference environment variables using the actual case on all platforms, and we can have an internal workaround (that we don't have to expose to the user) if it turns out that that doesn't work on Windows. It sounds like Rust on Windows already uppercases environment variable names, so wouldn't that Just Work? Cargo would set the environment variable by name (which gets internally uppercased on Windows), and rustc would read the environment variable by name (which gets internally uppercased on Windows), so they'd match and write/read the same environment variable.

@ehuss

This comment has been minimized.

Copy link
Contributor Author

ehuss commented Jan 15, 2020

I'll need to do some tests on Windows to see how it treats unicode. I know the Rust std lib only handles ascii, but I'm not sure what Windows itself does (I can't find any info online). I'll investigate and see what issues arise. In theory the rustc side should also be case-insensitive on windows so it shouldn't matter what case it is, but I suspect it is broken for unicode.

Are you aware of examples of tools with environment variables that use case-sensitive mixed-case variable names? It seems very unusual to me.

Programs distinguished only by case isn't a theoretical issue,

I think that's a bit of a stretch, this only matters when the programs are in the same Cargo workspace.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 15, 2020

@ehuss

I appreciate you looking into this further. Thank you.

Are you aware of examples of tools with environment variables that use case-sensitive mixed-case variable names? It seems very unusual to me.

Yes, typically for this exact reason: a fixed prefix followed by arbitrary user-defined data. Off the top of my head, I know autotools does this quite often.

I think that's a bit of a stretch, this only matters when the programs are in the same Cargo workspace.

  1. Consider a busybox-like project that implements many programs as part of the same project.

  2. Even if it doesn't cause breakage in a given project, it still leads to confusion and not a small amount of dismay to run into a program on Linux that changes the case of filenames, as people who have run into such issues before know the potential for such breakage.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 16, 2020

If env::var("CARGO_THING_the_binary") works on all platforms, that's fine by me. I don't mind whether it's uppercase or lowercase, but "the naive thing" should work on all platforms. I'm not sure whether that works on Windows or not, myself, though.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 17, 2020

@alexcrichton

If env::var("CARGO_THING_the_binary") works on all platforms, that's fine by me.

It should, as far as I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.