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

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

Merged
merged 7 commits into from
Jan 22, 2020

Conversation

Hoverbear
Copy link
Contributor

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:

https://github.com/timberio/vector/blob/b290ceb2aee4242834d434a4bd092fe2a0cece3b/lib/file-source/src/metadata_ext.rs#L23-L30

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!

@Hoverbear Hoverbear requested a review from LucioFranco January 21, 2020 21:50
@@ -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"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Hoverbear Hoverbear Jan 21, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thats fine, its windows anyways :)

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM!

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

@Hoverbear Hoverbear force-pushed the windows-stable branch 2 times, most recently from 7f2e7df to ec4379f Compare January 21, 2020 22:44
@Hoverbear
Copy link
Contributor Author

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

@Hoverbear
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ghost
Copy link

ghost 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
Copy link
Contributor

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

@Hoverbear
Copy link
Contributor Author

Let's have a try. :)

@ghost
Copy link

ghost 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
Copy link
Contributor

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

@Hoverbear
Copy link
Contributor Author

Absolutely!

@Hoverbear
Copy link
Contributor Author

@LucioFranco A final look over?

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

LGTM!

@LucioFranco LucioFranco merged commit e3afda3 into vectordotdev:master Jan 22, 2020
@binarylogic
Copy link
Contributor

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the stable rust compiler on windows
3 participants