Fix handlers_test linkage and windows failures #385
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piscisaureus
force-pushed
the
testlink
branch
6 times, most recently
from
81f254c to
83cab7b
Compare
ry
reviewed
Jul 19, 2018
| } | ||
| if (is_win) { | ||
| libs = [ "userenv.lib" ] | ||
| } |
There was a problem hiding this comment.
Are these not discovered with the get_rust_ldflags.py?
There was a problem hiding this comment.
They are, but test_cc still needs it here, because it's not linked with rust_executable.
| "$rust_build:idna", | ||
| "$rust_build:percent_encoding", | ||
| "$rust_build:unicode_bidi", | ||
| "$rust_build:unicode_normalization", |
There was a problem hiding this comment.
This is unfortunate. What if we moved all the tests into their own file - handlers_test.rs ?
There was a problem hiding this comment.
IDK - I thought of that too but I didn't try. I suspect its gonna be very brittle.
Another meh option is to generate a list of all crates in rust/BUILD.gn, and just pass that to all targets.
Maybe best to let it stew for a while and then ask some rust person for their advice.
There was a problem hiding this comment.
best to let it stew for a while
agree
Another meh option is to generate a list of all crates in rust/BUILD.gn
I believe fuchsia does something like this.
| @@ -0,0 +1,115 @@ | |||
| #!/usr/bin/env python | |||
| # Copyright 2018 Bert Belder <[email protected]> | |||
There was a problem hiding this comment.
(We should change to "Deno authors" soon. Maybe after we move out of github.com/ry)
Please add a big comment at the top of this file explaining the purpose of get_rust_ldflags.py ... I think this is likely to be confusing to future readers.
There was a problem hiding this comment.
Yeah I'll add a big comment to rust.gni and a small one here. You can't say this file is sparsely commented...
build_extra/rust/rust.gni
Outdated
| empty_rs_path, | ||
| "--test", | ||
| ], | ||
| "list lines") |
There was a problem hiding this comment.
list lines?
Please point the reader to the soon-to-be-written comment at the top of get_rust_ldflags.py
There was a problem hiding this comment.
gn help exec_script
build_extra/rust/rust.gni
Outdated
| output_file_rel = rebase_path(output_file, root_build_dir) | ||
| args += [ "--emit=$emit_type=$output_file_rel" ] | ||
|
|
||
| # TODO(ry) For unknown reasons emitting a depfile on tests doesn't work. |
It's unfortunate that it's now linking to files in |
|
They always were linked, you just didn't see it because rustc did it all
under the covers.
…On Thu, 19 Jul 2018 at 15:29, Ryan Dahl ***@***.***> wrote:
~/src/deno/build_extra/rust> ./get_rust_ldflags.py empty.rs
warning: `-C save-temps` might not produce all requested temporary products when incremental compilation is enabled.
-L
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib
empty.crate.allocator.rcgu.o
-L
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libstd-e2a1b7f1c041b010.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/liballoc_jemalloc-d3791a2de87534f9.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libpanic_unwind-1086e1d0e6cd90ef.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libunwind-ce923fc11d74f968.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/liballoc_system-e87ca03f4215e4ef.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/liblibc-e959e9c0d1548c0e.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/liballoc-37638280ec40d743.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libstd_unicode-0766335b25639e9e.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libcore-e1a6bc6fd1c79bbc.rlib
/Users/rld/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/lib/libcompiler_builtins-c410c86b18328824.rlib
-l
System
-l
resolv
-l
pthread
-l
c
-l
m
It's unfortunate that it's now linking to files in /Users/rld/.rustup/. I
suppose once we include rustc in third_party we can fix this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://github.com/ry/deno/pull/385#issuecomment-406276193>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AANUkQWKTGzepoJzOPzL5r1OX6RuKXKCks5uIInKgaJpZM4VV3P0>
.
|
ry
approved these changes
Jul 19, 2018
piscisaureus
pushed a commit
to piscisaureus/deno
that referenced
this pull request
Oct 7, 2019
hardfist
pushed a commit
to hardfist/deno
that referenced
this pull request
Aug 7, 2024
Bumped versions for 0.238.0 Please ensure: - [ ] Crate versions are bumped correctly To make edits to this PR: ```shell git fetch upstream release_0_238.0 && git checkout -b release_0_238.0 upstream/release_0_238.0 ``` cc @mmastrac Co-authored-by: mmastrac <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tested on linux and windows, but not on mac.
Fixes #380