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

Skip linker-output-non-utf8 test on Apple #47289

Merged
merged 3 commits into from
Jan 12, 2018
Merged

Skip linker-output-non-utf8 test on Apple #47289

merged 3 commits into from
Jan 12, 2018

Conversation

etaoins
Copy link
Contributor

@etaoins etaoins commented Jan 9, 2018

This test fails on APFS filesystems with the following error:

mkdir: /Users/ryan/Code/rust/build/x86_64-apple-darwin/test/run-make/linker-output-non-utf8.stage2-x86_64-apple-darwin/zzz�: Illegal byte sequence

The mkdir does succeed on an HFS+ volume mounted on the same system:

$ mkdir zzz$$'\xff'
$ ls
zzz47432\xff

This is due to APFS now requiring that all paths are valid UTF-8. As APFS will be the default filesystem for all new Darwin-based systems the most straightforward fix is to skip this test on Darwin as well as Windows.

This test fails on APFS filesystems with the following error:

mkdir: /Users/ryan/Code/rust/build/x86_64-apple-darwin/test/run-make/linker-output-non-utf8.stage2-x86_64-apple-darwin/zzz�: Illegal byte sequence

This is due to APFS now requiring that all paths are valid UTF-8. As
APFS will be the default filesystem for all new Darwin-based systems the
most straightforward fix is to skip this test on Darwin as well as
Windows.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.


else
# This does not work in its current form on Windows or Apple APFS due
# to their filesystems requiring paths to be valid Unicode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure Windows require paths to be valid Unicode? If not, please retain the original comment about Windows, and open a new sentence about the valid Unicode requirement with APFS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it needs to parse the path as Unicode to perform normalisation and case folding on NTFS and ReFS. MSDN covers this briefly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true - when MSDN refers to "unicode" it means un-validated UCS-2 - case mapping ignores invalid sequences. @retep998 probably knows more details.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows does not perform unicode normalization on paths. Case folding is also done via a highly simplistic u16 to u16 mapping which will handle lone surrogates just fine. Windows uses WTF-16 for paths, which is a superset of UTF-16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the correction. I'll switch the comment back.

@est31
Copy link
Member

est31 commented Jan 9, 2018

As APFS will be the default filesystem for all new Darwin-based systems

Don't want to be nitpicky but maybe this is the case for Darwin based operating systems from Apple but the APFS driver is not included in the Darwin kernel itself and Apple won't publish its source code, so most likely non-Apple Darwin distributions (like PureDarwin) won't get any APFS support soon, let alone APFS being the default filesystem. In fact it seems that Apple has removed the HFS driver from its kernel releases and moved it to its own project.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 9, 2018
@etaoins
Copy link
Contributor Author

etaoins commented Jan 9, 2018

@est31 That's fair. I was using Darwin to refer to the macOS/iOS/tvOS/watchOS family of operating systems but you're correct for other Darwin forks.

The fact that this test runs on Linux should be enough to ensure that the compiler is dealing with non-UTF-8 paths correctly internally. I don't think it's a major loss to not run this test on platforms that could potentially support it, especially considering the complexity of determining the target filesystem type.

The Windows situation is more complicated than I realised
@etaoins
Copy link
Contributor Author

etaoins commented Jan 9, 2018

@kennytm Restored the original comment


else
# This also does not work on Apple APFS due to the filesystem requiring
# valid UTF-u paths.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTF-u? 😏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@kennytm
Copy link
Member

kennytm commented Jan 10, 2018

Thank you!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 10, 2018

📌 Commit b69c320 has been approved by kennytm

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Jan 11, 2018
…-test-on-apple, r=kennytm

Skip linker-output-non-utf8 test on Apple

This test fails on APFS filesystems with the following error:

```shell
mkdir: /Users/ryan/Code/rust/build/x86_64-apple-darwin/test/run-make/linker-output-non-utf8.stage2-x86_64-apple-darwin/zzz�: Illegal byte sequence
```

The mkdir does succeed on an HFS+ volume mounted on the same system:
```shell
$ mkdir zzz$$'\xff'
$ ls
zzz47432\xff
```

This is due to APFS now requiring that all paths are valid UTF-8. As APFS will be the default filesystem for all new Darwin-based systems the most straightforward fix is to skip this test on Darwin as well as Windows.
kennytm added a commit to kennytm/rust that referenced this pull request Jan 11, 2018
…-test-on-apple, r=kennytm

Skip linker-output-non-utf8 test on Apple

This test fails on APFS filesystems with the following error:

```shell
mkdir: /Users/ryan/Code/rust/build/x86_64-apple-darwin/test/run-make/linker-output-non-utf8.stage2-x86_64-apple-darwin/zzz�: Illegal byte sequence
```

The mkdir does succeed on an HFS+ volume mounted on the same system:
```shell
$ mkdir zzz$$'\xff'
$ ls
zzz47432\xff
```

This is due to APFS now requiring that all paths are valid UTF-8. As APFS will be the default filesystem for all new Darwin-based systems the most straightforward fix is to skip this test on Darwin as well as Windows.
bors added a commit that referenced this pull request Jan 11, 2018
Rollup of 12 pull requests

- Successful merges: #47283, #47288, #47289, #47298, #47305, #47307, #47310, #47322, #47324, #47328, #47340, #47344
- Failed merges:
bors added a commit that referenced this pull request Jan 12, 2018
Rollup of 12 pull requests

- Successful merges: #47283, #47288, #47289, #47298, #47305, #47307, #47310, #47322, #47324, #47328, #47340, #47344
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Jan 12, 2018
…-test-on-apple, r=kennytm

Skip linker-output-non-utf8 test on Apple

This test fails on APFS filesystems with the following error:

```shell
mkdir: /Users/ryan/Code/rust/build/x86_64-apple-darwin/test/run-make/linker-output-non-utf8.stage2-x86_64-apple-darwin/zzz�: Illegal byte sequence
```

The mkdir does succeed on an HFS+ volume mounted on the same system:
```shell
$ mkdir zzz$$'\xff'
$ ls
zzz47432\xff
```

This is due to APFS now requiring that all paths are valid UTF-8. As APFS will be the default filesystem for all new Darwin-based systems the most straightforward fix is to skip this test on Darwin as well as Windows.
bors added a commit that referenced this pull request Jan 12, 2018
@bors bors merged commit b69c320 into rust-lang:master Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants