make ignore-git true by default when channel is dev #43895

JeremySorensen · 2017-08-16T01:42:59Z

Fixes #43771
(Handle git info in rustbuild differently)

rust-highfive · 2017-08-16T01:43:12Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

JeremySorensen · 2017-08-16T01:46:55Z

This is my first PR, so please tell me what I did wrong!
I wanted to point out that I noticed that the default channel is "dev" when nothing is specified, so I changed git-ignore to true by default to be consistent. (Both in the code and in the config.toml.example file.)

Mark-Simulacrum · 2017-08-16T04:14:51Z

Thanks! If you add "Fixes #43771" to the description (remove issue) GitHub will automatically close it once this merges.

@bors r+

bors · 2017-08-16T04:14:52Z

📌 Commit c3b27d5 has been approved by Mark-Simulacrum

ishitatsuyuki · 2017-08-16T06:06:18Z

I have no idea why you're doing this, as almost any action will cause a full rebuild due to the staged nature.

This removes the version number from my dev channel build, and I will have to change the channel to nightly as the ./configure script doesn't have option related to git.

Oh, and by the way, dev is the default with git, and stable is the default without git. I think a build should contain version number out of box. If you need to disable for timestamp reasons, just do in your own config.toml.

bors · 2017-08-16T06:43:23Z

⌛ Testing commit c3b27d5 with merge b7afe1fdef0a39be24a9fee3bd782e370e2409f6...

ishitatsuyuki · 2017-08-16T06:45:38Z

Oh, I see, there was a discussion for this decision. Nevermind my comments.

bors · 2017-08-16T06:56:08Z

💔 Test failed - status-travis

kennytm · 2017-08-16T07:06:51Z

@bors: retry

distcheck failed, feels spurious.

[00:07:21] downloading https://static.rust-lang.org/dist/2017-07-18/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
######################################################################## 100.0%
[00:07:22] extracting /checkout/obj/build/tmp/distcheck/build/cache/2017-07-18/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:07:22] error: failed to load source for a dependency on `serde_derive`
[00:07:22] 
[00:07:22] Caused by:
[00:07:22]   Unable to update registry https://github.com/rust-lang/crates.io-index
[00:07:22] 
[00:07:22] Caused by:
[00:07:22]   failed to update replaced source `registry https://github.com/rust-lang/crates.io-index`
[00:07:22] 
[00:07:22] Caused by:
[00:07:22]   failed to read root of directory source: /checkout/obj/build/tmp/distcheck/src/vendor
[00:07:22] 
[00:07:22] Caused by:
[00:07:22]   No such file or directory (os error 2)
[00:07:22] failed to run: /checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/obj/build/tmp/distcheck/src/bootstrap/Cargo.toml --locked --frozen
[00:07:22] Build completed unsuccessfully in 0:00:09
[00:07:22] Makefile:54: recipe for target 'check' failed

bors · 2017-08-17T08:54:56Z

⌛ Testing commit c3b27d5 with merge 2c5f1bc088de952ace86da838b963711357e5880...

bors · 2017-08-17T09:06:13Z

💔 Test failed - status-travis

TimNN · 2017-08-17T09:31:06Z

distcheck failed again, I don't think this is spurious. Looking at the patch, I'm not sure if it entirely correct, although I'm not currently that familiar with the inner working of rustbuild:

This patch changes the default of ignore-git to true and then later updates the default based on channel, but only if config.toml contains a [rust] section. Given that the builders still use config.mk I believe that ignore-git will now be true on all builders, which then causes the above failure.

TimNN · 2017-08-17T09:31:37Z

config.toml.example

@@ -259,7 +259,10 @@
 #codegen-tests = true

 # Flag indicating whether git info will be retrieved from .git automatically.
-#ignore-git = false
+# Having the git information can cause a lot of rebuilds during development.
+# Note: If this attribute is not explicity set (e.g. it left commented out) it


This should probably be "if left commented out".

yes you are right

JeremySorensen · 2017-08-17T14:15:21Z

Given that the builders still use config.mk I believe that ignore-git will now be true on all builders, which then causes the above failure.

Oh darn... OK well I think the fix is simple, just revert the initial value of ignore-git back to false, and only keep the logic for setting the default if there is a config.toml file.
But that raises a question, are there some tests that:

Use a config.toml file
Do NOT specify an explicit value for ignore-git
Require ignore-git to be false in order to pass?

If all of those are true, for any test, then that(those) test(s) will fail. So if we want to keep this patch, we would need to find all such tests and explicitly set ignore-git to false.

JeremySorensen · 2017-08-23T02:57:09Z

sorry I was trying to figure out how to update a pull request, I didn't realize that pushing to my fork was enough to cause stuff to run. I will follow contributing.md more closely and see if I can figure out what is going on with the tests.

JeremySorensen · 2017-08-24T04:31:33Z

OK I am getting to the point where I need some mentoring here. When I ran the tests (on my machine), a whole block of them failed with the same error. Here is an excerpt of the results:

    Check compiletest suite=run-make mode=run-make (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
    Building rustdoc for stage2 (x86_64-pc-windows-msvc)

    running 159 tests
    test [run-make] run-make\allow-non-lint-warnings-cmdline ... FAILED
    test [run-make] run-make\alloc-extern-crates ... FAILED
    ...
    test [run-make] run-make\windows-subsystem ... FAILED

    failures:

    ---- [run-make] run-make\allow-non-lint-warnings-cmdline stdout ----
    	thread '[run-make] run-make\allow-non-lint-warnings-cmdline' panicked at 'failed to spawn `make`:     Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4
    note: Run with `RUST_BACKTRACE=1` for a backtrace.

    ---- [run-make] run-make\alloc-extern-crates stdout ----
    	thread '[run-make] run-make\alloc-extern-crates' panicked at 'failed to spawn `make`: Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4
    ...
    ---- [run-make] run-make\windows-subsystem stdout ----
	thread '[run-make] run-make\windows-subsystem' panicked at 'failed to spawn `make`: Error { repr: Os { code: 2, message: "The system cannot find the file specified." } }', src\libcore\result.rs:860:4

    failures:
        [run-make] run-make\a-b-a-linker-guard
        [run-make] run-make\alloc-extern-crates
    ...
        [run-make] run-make\windows-subsystem

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

But the thing is I get this even after I revert back to the original config.rs. So the only thing that is changed is config.toml, but everything builds fine its the tests that fail. So I am not sure why these tests are failing but it doesn't seem related to my code.

Also I am not sure why the travis-ci failed, it kind of looks like it got cancelled, if somebody cancelled it because of my comment indicating that I had kicked it off inadvertently then that is fine, but I don't know how to tell that.

At this point the only change to the code is one line:
config.ignore_git = config.channel == "dev";
which goes right before this existing line:
set(&mut config.ignore_git, rust.ignore_git);
So I think that is right, if the travis-ci was cancelled, can restart it and rerun the tests on the server to see which fail?
Also how can I get those tests to pass? Should I just do a clean clone of my fork and start over?

Mark-Simulacrum · 2017-08-24T22:23:13Z

It looks like you might not have make installed. Maybe try running that and see if it exists? Travis passed, and I think the code is correct, so:

@bors r+

bors · 2017-08-24T22:23:13Z

📌 Commit bdb7901 has been approved by Mark-Simulacrum

kennytm · 2017-08-29T06:33:10Z

Unable to update registry, spurious. But @JeremySorensen wants to hold off a bit?

alexcrichton · 2017-08-29T14:29:49Z

@JeremySorensen oh bors is our integration bot, when we "r+" a PR it goes into a queue of PRs to be tested, and then the queue is drained one at a time. You've also now seen what happens when tests fail! (spuriously)

The comment looks fine by me though, but @JeremySorensen did you want to reword it?

JeremySorensen · 2017-08-29T15:00:10Z

@alexcrichton I think the expected behavior is that if the user doesn't copy config.toml.example over, OR if they do but don't change it, then the channel will be "dev" and ignore_git will be true correct? If so, then yes the comment looks correct to me. (I thought I remembered it was still set to false in the example, but I was wrong.)

alexcrichton · 2017-08-29T15:15:17Z

Ok!

I think, though, that the failure here is not a spurious failure. I believe what you'll want to do is add an option to the configure script for this one builder. You can find the builder's configuration in src/ci/docker/x86_64-gnu-distcheck and the env var should be something like RUST_CONFIGURE_ARGS. The argument you want to add is --set build.ignore-git=true

Note that --set hasn't been tested much before, so there may be a bug or two with that...

alexcrichton · 2017-08-29T16:03:16Z

Er sorry, you want ignore-git=false b/c we want that builder to read git information!

alexcrichton · 2017-08-29T17:40:00Z

@bors: r+

bors · 2017-08-29T17:40:01Z

📌 Commit d24ee23 has been approved by alexcrichton

arielb1 · 2017-08-29T21:41:22Z

configure: processing command line

[00:00:27] configure: 

[00:00:27] configure: rust.debug-assertions := True

[00:00:27] configure: build.ignore-git     := false

[00:00:27] configure: build.submodules     := False

[00:00:27] configure: llvm.assertions      := True

[00:00:27] configure: build.locked-deps    := True

[00:00:27] configure: llvm.ccache          := sccache

[00:00:27] configure: build.openssl-static := True

[00:00:27] configure: build.build          := x86_64-unknown-linux-gnu

[00:00:27] configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--set',  ...

[00:00:27] Traceback (most recent call last):

[00:00:27]   File "/checkout/src/bootstrap/configure.py", line 378, in <module>

[00:00:27]     configure_section(sections[section_key], section_config)

[00:00:27]   File "/checkout/src/bootstrap/configure.py", line 367, in configure_section

[00:00:27]     raise RuntimeError("failed to find config line for {}".format(key))

[00:00:27] RuntimeError: failed to find config line for ignore-git

@bors r-

alexcrichton · 2017-08-29T22:04:40Z

I think maybe rust.ignore-git? @JeremySorensen I'm shooting the dark here so it'd be best if you could verify locally, which I believe you can do via ./src/ci/docker/run.sh x86_64-gnu-distcheck

JeremySorensen · 2017-08-29T22:50:17Z

I ran it as is and got the same error (as expected), then I changed the line in Dockerfile to
ENV RUST_CONFIGURE_ARGS --build=x86_64-unknown-linux-gnu --set rust.ignore-git=false
now I get this:

failed to parse TOML configuration 'config.toml': invalid type: string "false", expected a boolean for key rust.ignore-git

I checked, the false is not in quotes in the Docker file, so I guess it is getting escaped incorrectly, or not getting parsed.

alexcrichton · 2017-08-29T22:54:09Z

I think we may need to add a special case to this line to just convert "true" to True to get properly set as a boolean perhaps?

JeremySorensen · 2017-08-29T23:11:58Z

@alexcrichton Ha! I just did that! my code looks like this:

for key in known_args:
    # The `set` option is special and can be passed a bunch of times
    if key == 'set':
        for option, value in known_args[key]:
            keyval = value.split('=', 1)
            if len(keyval) == 1 or keyval[1] == "true":
                value = True
            elif keyval[1] == "false":
                value = False
            else:
                value = keyval[1]
            set(keyval[0], value)
        continue

We actually need the false case here
Should I commit that for this PR as well?
edit: just noticed we seem to use single quotes for strings in Python, I can fix that before I commit.
edit 2: nevermind we use double quotes in lots of other places, not sure why 'set' is in single quotes

alexcrichton · 2017-08-29T23:17:26Z

Looks good to me!

JeremySorensen · 2017-08-30T04:56:05Z

I ran the tests again and it looks like it got the ignore-git=false, but I am having a bunch of seemingly unrelated errors. I don't know anything about Docker (I had to install it to make the command work) but I wonder if it is because I ran the test several times and killed it in progress a couple times, or if somehow I really broke everything. Should I push the changes to the Docker file and configure.py?

I didn't redirect the output of the command to a file and it rolled off the end so I don't know if I got everything but here are at least some of the errors:

get_toml (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_toml ... ok
program_config (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.program_config ... FAIL
rustc_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.rustc_stamp ... ok
...
Return True when the stamp file does not exists ... ok

======================================================================
FAIL: program_config (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.program_config
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/doctest.py", line 2226, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for bootstrap.RustBuild.program_config
  File "/checkout/src/bootstrap/bootstrap.py", line 519, in program_config

----------------------------------------------------------------------
File "/checkout/src/bootstrap/bootstrap.py", line 527, in bootstrap.RustBuild.program_config
Failed example:
    cargo_path.rstrip(".exe") == os.path.join("/tmp/rust",
    "bin", "cargo")
Expected:
    True
Got:
    False


----------------------------------------------------------------------
Ran 14 tests in 0.068s

FAILED (failures=1)
invalid checksum:
    found:    334d016f755cd6dc58c53a86e183882f8ec14f52fb05345887c8a5edd42c87b7
    expected: 64ec88ca00b268e5ba1a35678a1b5316d212f4f366b2477232534a8aeca37f3c
+ python2.7 ../x.py test distcheck
    Finished dev [unoptimized] target(s) in 0.0 secs
Distcheck
...
failures:
    net::tcp::tests::clone_accept_concurrent
    net::tcp::tests::clone_accept_smoke
    net::tcp::tests::clone_while_reading
    net::tcp::tests::close_read_wakes_up
    net::tcp::tests::close_readwrite_smoke
    net::tcp::tests::connect_loopback
    net::tcp::tests::double_bind
    net::tcp::tests::fast_rebind
    net::tcp::tests::multiple_connect_interleaved_greedy_schedule
    net::tcp::tests::multiple_connect_interleaved_lazy_schedule
    net::tcp::tests::multiple_connect_serial
    net::tcp::tests::partial_read
    net::tcp::tests::peek
    net::tcp::tests::read_eof
    net::tcp::tests::shutdown_smoke
    net::tcp::tests::smoke_test
    net::tcp::tests::socket_and_peer_name
    net::tcp::tests::tcp_clone_smoke
    net::tcp::tests::tcp_clone_two_read
    net::tcp::tests::tcp_clone_two_write
    net::tcp::tests::write_close
    net::udp::tests::connect_send_peek_recv
    net::udp::tests::peek_from
    net::udp::tests::set_nonblocking
    net::udp::tests::socket_name_ip4
    net::udp::tests::socket_smoke_test_ip4
    net::udp::tests::udp_clone_smoke
    net::udp::tests::udp_clone_two_read
    net::udp::tests::udp_clone_two_write

test result: FAILED. 773 passed; 29 failed; 0 ignored; 0 measured; 0 filtered out

alexcrichton · 2017-08-30T05:02:27Z

Yeah that all looks fine to me, thanks for investigating though! I'm not sure what's up with the python looking failure, but the docker failures I believe are all related to IPv6 in the container. I get them locally as well and I'm not sure how to fix them :(

alexcrichton · 2017-08-30T14:38:32Z

@bors: r+

Thanks!

bors · 2017-08-30T14:38:33Z

📌 Commit 4f591a4 has been approved by alexcrichton

bors · 2017-08-30T15:43:16Z

⌛ Testing commit 4f591a4 with merge 51a54b6...

make ignore-git true by default when channel is dev Fixes #43771 (Handle git info in rustbuild differently)

bors · 2017-08-30T18:34:52Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 51a54b6 to master...

make ignore-git true by default when channel is dev

c3b27d5

rust-highfive assigned brson Aug 16, 2017

Mark-Simulacrum assigned Mark-Simulacrum and unassigned brson Aug 16, 2017

aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 16, 2017

TimNN reviewed Aug 17, 2017

View reviewed changes

JeremySorensen added 2 commits August 17, 2017 07:26

fix typo in git-ignore comment

5e472c2

initialize ignore-git to false

bdb7901

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 22, 2017

Mark-Simulacrum added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 24, 2017

add ignore-git=true option to RUST_CONFIGURE_ARGS

68aab69

change option for RUST_CONFIGURE_ARGS to ignore-git=false

d24ee23

arielb1 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2017

arielb1 mentioned this pull request Aug 29, 2017

Rollup of 13 pull requests #44166

Closed

JeremySorensen added 2 commits August 29, 2017 21:57

allow value of key/value pair argument to set option be boolean

873a05e

fix option for RUST_CONFIGURE_ARGS to be rust.ignore-git=false

4f591a4

bors added a commit that referenced this pull request Aug 30, 2017

Auto merge of #43895 - JeremySorensen:master, r=alexcrichton

51a54b6

make ignore-git true by default when channel is dev Fixes #43771 (Handle git info in rustbuild differently)

bors merged commit 4f591a4 into rust-lang:master Aug 30, 2017

make ignore-git true by default when channel is dev #43895

make ignore-git true by default when channel is dev #43895

Conversation

JeremySorensen commented Aug 16, 2017 • edited Loading

rust-highfive commented Aug 16, 2017

JeremySorensen commented Aug 16, 2017

Mark-Simulacrum commented Aug 16, 2017

bors commented Aug 16, 2017

ishitatsuyuki commented Aug 16, 2017

bors commented Aug 16, 2017

ishitatsuyuki commented Aug 16, 2017

bors commented Aug 16, 2017

kennytm commented Aug 16, 2017 • edited Loading

bors commented Aug 17, 2017

bors commented Aug 17, 2017

TimNN commented Aug 17, 2017

TimNN Aug 17, 2017

Choose a reason for hiding this comment

JeremySorensen Aug 17, 2017

Choose a reason for hiding this comment

JeremySorensen commented Aug 17, 2017

JeremySorensen commented Aug 23, 2017

JeremySorensen commented Aug 24, 2017

Mark-Simulacrum commented Aug 24, 2017

bors commented Aug 24, 2017

kennytm commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

JeremySorensen commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

bors commented Aug 29, 2017

arielb1 commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

JeremySorensen commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

JeremySorensen commented Aug 29, 2017 • edited Loading

alexcrichton commented Aug 29, 2017

JeremySorensen commented Aug 30, 2017

alexcrichton commented Aug 30, 2017

alexcrichton commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

bors commented Aug 30, 2017

JeremySorensen commented Aug 16, 2017 •

edited

Loading

kennytm commented Aug 16, 2017 •

edited

Loading

JeremySorensen commented Aug 29, 2017 •

edited

Loading