Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

feat(Windows platform): Allow building on Windows Stable Rust #1560

Merged
merged 7 commits into from Jan 22, 2020

Conversation

@Hoverbear
Copy link
Contributor

Hoverbear commented Jan 21, 2020

Support building on Windows with rustc stable.

This PR fixes #1480 by importing a few pieces of stdlib code.

In #1480 @LucioFranco and I discussed some reasoning about why the code is here and not another module. In short: It's simpler! I'd be happy to make it a separate mod though.

The only usages were:

impl PortableMetadataExt for Metadata {
fn portable_dev(&self) -> u64 {
self.volume_serial_number().unwrap_or(0u32) as u64
}
fn portable_ino(&self) -> u64 {
self.file_index().unwrap_or(0u64)
}
}

Implementation notes

It was a small conundrum since Metadata doesn't have this information without the nightly feature. With that feature enabled this following code builds the datum which the functions we want read from:

https://github.com/rust-lang/rust/blob/30ddb5a8c1e85916da0acdc665d6a16535a12dd6/src/libstd/sys/windows/fs.rs#L326-L351

So the easiest solution was to have the consumer code in file_server.rs and file_watcher.rs work with the file handle instead of the metadata, and then extend the File type.

Caveats:

  • Since it's possible this code will be updated upstream in the future, as few changes as possible were made, allowing warnings that already existing in the code to continue to exist.
  • Added a big FIXME tag to the file to make sure folks can know why it's there.
  • Winapi/libc are unfortunate dependencies to add. I think we can find a way to be clever and avoid these impacting other things if you'd like.
  • It's a hack!
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear requested a review from LucioFranco Jan 21, 2020
@@ -14,6 +14,8 @@ scan_fmt = "0.2.3"
tracing = "0.1.2"
indexmap = {version = "1.0.2", features = ["serde-1"]}
flate2 = "1.0.6"
winapi = { version = "0.3.8", features = ["winioctl"] }

This comment has been minimized.

Copy link
@LucioFranco

LucioFranco Jan 21, 2020

Member

I think we pullin winapi 0.2 via mio 0.6 to avoid a duplicate here for now, is it possible to use winapi 0.2? I don't think this should hurt compile times either since we already need to compile it.

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear Jan 21, 2020

Author Contributor

Fantastic, great catch. :) I'll verify this and make the change.

This comment has been minimized.

Copy link
@Hoverbear

Hoverbear Jan 21, 2020

Author Contributor

Seems winapi 2.x doesn't have some variables like FSCTL_GET_REPARSE_POINT. :(

This comment has been minimized.

Copy link
@LucioFranco

LucioFranco Jan 21, 2020

Member

thats fine, its windows anyways :)

Copy link
Member

LucioFranco left a comment

LGTM!

cc @lukesteensen @a-rodin you both might be interested in this change.

@Hoverbear Hoverbear force-pushed the Hoverbear:windows-stable branch 2 times, most recently from 7f2e7df to ec4379f Jan 21, 2020
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear force-pushed the Hoverbear:windows-stable branch from ec4379f to cb9dee6 Jan 21, 2020
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear force-pushed the Hoverbear:windows-stable branch from cb9dee6 to d4af4c3 Jan 21, 2020
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 21, 2020

I think 3388a74 is enough to get Windows on CI to use stable now.

@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 21, 2020

Hm, I'm not sure if this is a related failure: https://circleci.com/gh/timberio/vector/78014

     Running target/debug/deps/crash-6538aa49a658be1b

running 4 tests
test test_source_panic ... FAILED
test test_source_error ... ok
test test_sink_panic ... ok
test test_sink_error ... ok

failures:

failures:
    test_source_panic

test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test crash'
builder for '/nix/store/mk7mmp05sfg9l6wk46542y6xmz4b12sr-vector-0.7.0.drv' failed with exit code 101
error: build of '/nix/store/mk7mmp05sfg9l6wk46542y6xmz4b12sr-vector-0.7.0.drv' failed
@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 21, 2020

I am similarly confused by: https://circleci.com/gh/timberio/vector/78008

test parses_sink_full_es_aws ... FAILED
test parses_sink_full_es_basic_auth ... ok
test parses_sink_full_request ... ok
test parses_sink_no_request ... ok
test parses_sink_partial_request ... ok
test warnings ... ok

failures:

---- parses_sink_full_es_aws stdout ----
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ["Sink \"out\": Could not generate AWS credentials: CredentialsError { message: \"Couldn\\\'t find AWS credentials in environment, credentials file, or IAM role.\" }"]', src\libcore\result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.


failures:
    parses_sink_full_es_aws

test result: FAILED. 21 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test config'

Exited with code exit status 101
@a-rodin

This comment has been minimized.

Copy link
Member

a-rodin commented Jan 22, 2020

Hm, I'm not sure if this is a related failure: https://circleci.com/gh/timberio/vector/78014

I ran this check locally and it passed. Sometimes the CI instance turns out to be too slow, which makes this test fail. Restarting the job might help.

I am similarly confused by: https://circleci.com/gh/timberio/vector/78008

It is a known issue, see #1527. It is safe to ignore it for now as it is not related to the changes.

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Jan 22, 2020

I think this just needs a rebase/merge of master?

@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 22, 2020

Let's have a try. :)

@a-rodin

This comment has been minimized.

Copy link
Member

a-rodin commented Jan 22, 2020

Just a note: we probably would need to update the instructions for building Vector on Windows to use stable Rust. These instructions can be updated by changing the template and running make generate (or ./scripts/docker-run.sh checker make generate to run the generation using Docker). However, it doesn't block merging this PR because the instructions using nightly Rust should work with code targeting stable as well.

@LucioFranco

This comment has been minimized.

Copy link
Member

LucioFranco commented Jan 22, 2020

@Hoverbear would you mind doing what @a-rodin said and then we can merge? Thanks!

@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 22, 2020

Absolutely!

@Hoverbear Hoverbear requested a review from binarylogic as a code owner Jan 22, 2020
@Hoverbear

This comment has been minimized.

Copy link
Contributor Author

Hoverbear commented Jan 22, 2020

@LucioFranco A final look over?

Copy link
Member

LucioFranco left a comment

LGTM!

@LucioFranco LucioFranco merged commit e3afda3 into timberio:master Jan 22, 2020
12 checks passed
12 checks passed
Header rules No header rules processed
Details
Pages changed 143 new files uploaded
Details
Redirect rules No redirect rules processed
Details
DCO DCO
Details
Mixed content No mixed content detected
Details
Semantic Pull Request ready to be squashed
Details
ci/circleci: check-code Your tests passed on CircleCI!
Details
ci/circleci: check-fmt Your tests passed on CircleCI!
Details
ci/circleci: check-generate Your tests passed on CircleCI!
Details
ci/circleci: test-stable Your tests passed on CircleCI!
Details
ci/circleci: test-stable-kubernetes Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
@binarylogic

This comment has been minimized.

Copy link
Member

binarylogic commented Jan 22, 2020

Nice work!

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