Fix `build.rs`: Use `env::var_os` to retrieve `Path` #1580

NobodyXu · 2023-01-28T01:51:07Z

Fix retrieveing OUT_DIR in build.rs
Fix getting CARGO_MANIFEST_DIR: Use env::var_os
Modify run_command_with_args to take OsStr instead of String for args
Call env::var_os in read_env_var
Change return type to PathBuf instead of String for fn compile

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

briansmith · 2023-02-15T16:43:37Z

Is the entire purpose of this PR to support non-Unicode(-compatible) paths? Is there any other goal with this PR?

briansmith · 2023-02-15T21:57:47Z

I agree with the idea that we should be using OsStrings for paths. Do you have a realistic example of a configuration that works with this PR but doesn't work without this PR, so I can understand the real-world implications better? Thanks in advance!

NobodyXu · 2023-02-16T00:50:44Z

Is the entire purpose of this PR to support non-Unicode(-compatible) paths? Is there any other goal with this PR?

Yes

I agree with the idea that we should be using OsStrings for paths. Do you have a realistic example of a configuration that works with this PR but doesn't work without this PR, so I can understand the real-world implications better? Thanks in advance!

Linux filename can be anything that is \0 terminated and various unix program accepts anything that is ascii-compatible, that is, encoded in bytes and can use all ascii characters.

So the user can use arbitary non-unicode characters in their home dir or whatever dir, cargo new --lib . a project, then add this project as a dependency and tries to cargo b it.

Without non-unicode support, this will fail since the CARGO_MANIFEST_DIR is likely to be a absolute path.
It is also possible that OUR_DIR is a absolute path and even if it's not, the user could change the location and name of the target dir.

So it's possible for these environment variables to contain non-unicode and I'd like ring's build.rs to properly handle it.

briansmith · 2023-02-16T21:17:23Z

Linux filename can be anything that is \0 terminated and various unix program accepts anything that is ascii-compatible, that is, encoded in bytes and can use all ascii characters.

That is true, but if somebody is taking full advantage of that flexibility, it's really their own fault.

Without non-unicode support, this will fail since the CARGO_MANIFEST_DIR is likely to be a absolute path.

I understand. I'm simply asking could you provide a plausible scenerio where CARGO_MANIFEST_DIR contains non-Unicode-compatible characters? For example, "If a user with username "X" tries to build ring when their system character set is Y then the build fails." I.e. something I could test out myself without feeling silly.

NobodyXu · 2023-02-17T01:19:24Z

I understand. I'm simply asking could you provide a plausible scenerio where CARGO_MANIFEST_DIR contains non-Unicode-compatible characters? For example, "If a user with username "X" tries to build ring when their system character set is Y then the build fails." I.e. something I could test out myself without feeling silly.

On linux, I am able to create a non-unicode dir by running:

mkdir $(printf '\xFF')

And then cd into it by running:

cd $(printf '\xFF')

I can create a user with non-unicode home:

adduser a --home /home/"$(printf 'a\xFF')"

and cd into it:

cd /home/"$(printf 'a\xFF')"

briansmith · 2023-02-17T21:30:05Z

@NobodyXu Is this an issue that's causing trouble on a real system, or just a theoretical issue? 0xFF in a path is a theoretical issue. This will help me prioritize this.

NobodyXu · 2023-02-18T00:55:05Z

@NobodyXu Is this an issue that's causing trouble on a real system, or just a theoretical issue? 0xFF in a path is a theoretical issue. This will help me prioritize this.

Unfortunately I don't have any real world example for this, so right now it just stays as a theoretical issue, but I won't be surprised if somebody out there is having non-unicode path in their system, hence I think it's better to support any Path.

briansmith

PTAL at #1698. Your changes here are more thorough #1698. I think it would be great if you could review #1698 and then rebase this on that. If you do I will review it and merge it right away.

briansmith · 2023-10-06T03:21:48Z

build.rs

 }

 fn main() {
    const RING_PREGENERATE_ASM: &str = "RING_PREGENERATE_ASM";
    match read_env_var(RING_PREGENERATE_ASM).as_deref() {
-        Ok("1") => {
+        Some(val) if val == OsStr::new("1") => {


You can do Some(val) if val == "1"; see #1698.

briansmith · 2023-10-06T03:37:26Z

build.rs

@@ -692,9 +689,10 @@ fn nasm(file: &Path, arch: &str, include_dir: &Path, out_file: &Path) -> Command
    c
 }

-fn run_command_with_args<S>(command_name: S, args: &[String])
+fn run_command_with_args<S, Arg>(command_name: S, args: impl IntoIterator<Item = Arg>)


We should just get rid of the generics here; see #1698.

Use `env::var_os` instead of `env::var` to handle non utf-8 path correctly. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Do not convert `Path`/`OsStr` to `String` in `cc`, instead creates an `OsString` and push to it. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2023-10-06T11:28:48Z

@briansmith I've rebased this PR against main, please review this.

briansmith

Thank you for doing this. If you have other things you want to clean up in build.rs I would be happy to work with you on them. PR #1699 is another one you may be interested in.

briansmith · 2023-10-06T15:38:05Z

build.rs

+    target: &Target,
+    include_dir: &Path,
+    out_dir: &Path,
+) -> PathBuf {


briansmith · 2023-10-06T15:38:17Z

build.rs

-            out_file.to_str().expect("Invalid path")
-        ))
-        .arg(file);
+    let _ = c.arg("-c").arg(arg).arg(file);


Also very nice.

codecov · 2023-10-06T15:41:15Z

Codecov Report

Merging #1580 (dd3d803) into main (244a1de) will decrease coverage by 0.01%.
Report is 4 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1580      +/-   ##
==========================================
- Coverage   95.99%   95.98%   -0.01%     
==========================================
  Files         134      134              
  Lines       19980    19980              
  Branches      199      199              
==========================================
- Hits        19179    19178       -1     
  Misses        765      765              
- Partials       36       37       +1

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

briansmith · 2023-10-06T17:06:30Z

Thank you!

briansmith mentioned this pull request Oct 6, 2023

build.rs: Use OsStr/OsString more to handle non-Unicode paths. #1698

Merged

briansmith requested changes Oct 6, 2023

View reviewed changes

NobodyXu added 4 commits October 6, 2023 22:09

Fix retrieveing OUT_DIR in build.rs

5d7e6e6

Use `env::var_os` instead of `env::var` to handle non utf-8 path correctly. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix getting CARGO_MANIFEST_DIR: Use env::var_os

1f73f62

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Ret PathBuf instead of String for fn compile

b2252a8

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use Path::display instead of Path::to_str in fn cc

a861aa4

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu force-pushed the fix/out-dir branch from a756beb to a861aa4 Compare October 6, 2023 11:14

Fix fn cc in build.rs

dd3d803

Do not convert `Path`/`OsStr` to `String` in `cc`, instead creates an `OsString` and push to it. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested a review from briansmith October 6, 2023 11:28

briansmith approved these changes Oct 6, 2023

View reviewed changes

briansmith merged commit 2e06558 into briansmith:main Oct 6, 2023
139 of 140 checks passed

briansmith mentioned this pull request Oct 6, 2023

Remove the buildpath from debug info in files compiled by gcc and clang. #802

Open

NobodyXu deleted the fix/out-dir branch October 7, 2023 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `build.rs`: Use `env::var_os` to retrieve `Path` #1580

Fix `build.rs`: Use `env::var_os` to retrieve `Path` #1580

NobodyXu commented Jan 28, 2023

briansmith commented Feb 15, 2023

briansmith commented Feb 15, 2023

NobodyXu commented Feb 16, 2023

briansmith commented Feb 16, 2023

NobodyXu commented Feb 17, 2023 •

edited

Loading

briansmith commented Feb 17, 2023

NobodyXu commented Feb 18, 2023

briansmith left a comment

briansmith Oct 6, 2023

briansmith Oct 6, 2023

NobodyXu commented Oct 6, 2023

briansmith left a comment

briansmith Oct 6, 2023

briansmith Oct 6, 2023

codecov bot commented Oct 6, 2023 •

edited

Loading

briansmith commented Oct 6, 2023

Fix build.rs: Use env::var_os to retrieve Path #1580

Fix build.rs: Use env::var_os to retrieve Path #1580

Conversation

NobodyXu commented Jan 28, 2023

briansmith commented Feb 15, 2023

briansmith commented Feb 15, 2023

NobodyXu commented Feb 16, 2023

briansmith commented Feb 16, 2023

NobodyXu commented Feb 17, 2023 • edited Loading

briansmith commented Feb 17, 2023

NobodyXu commented Feb 18, 2023

briansmith left a comment

Choose a reason for hiding this comment

briansmith Oct 6, 2023

Choose a reason for hiding this comment

briansmith Oct 6, 2023

Choose a reason for hiding this comment

NobodyXu commented Oct 6, 2023

briansmith left a comment

Choose a reason for hiding this comment

briansmith Oct 6, 2023

Choose a reason for hiding this comment

briansmith Oct 6, 2023

Choose a reason for hiding this comment

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

briansmith commented Oct 6, 2023

Fix `build.rs`: Use `env::var_os` to retrieve `Path` #1580

Fix `build.rs`: Use `env::var_os` to retrieve `Path` #1580

NobodyXu commented Feb 17, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading