bootstrap: Add doctests and unitests #43046

milmazz · 2017-07-04T03:43:47Z

This commit includes the following changes:

Include more docstrings in classes, methods, and functions
Add doctests, which are great for self-documenting our source code
Add some unit tests with the unittest module
Remove WindowsError reference on non-windows systems
Rename some variables to be more explicit about their meaning
Move all the attributes defined outside of __init__
Add initial support for Python 3

rust-highfive · 2017-07-04T03:44:01Z

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.

milmazz · 2017-07-04T22:40:03Z

In this commit 26885c8 you'll find initial support for Python 3. I did some tests with tox including the current unit tests for Python 2.7, 3.5, and 3.6 and everything seems to be working:

$ cd src/bootstrap
$ tox
GLOB sdist-make: /Development/rust/rust/src/bootstrap/setup.py
py27 create: /Development/rust/rust/src/bootstrap/.tox/py27
py27 inst: /Development/rust/rust/src/bootstrap/.tox/dist/rust-bootstrap-0.0.1.zip
py27 installed: rust-bootstrap==0.0.1
py27 runtests: PYTHONHASHSEED='3410238989'
py27 runtests: commands[0] | /Development/rust/rust/src/bootstrap/.tox/py27/bin/python bootstrap_test.py
bin_root (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bin_root ... ok
bootstrap_binary (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bootstrap_binary ... ok
cargo_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.cargo_stamp ... ok
get_mk (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_mk ... ok
get_string (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_string ... ok
get_toml (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_toml ... ok
rustc_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.rustc_stamp ... ok
format_build_time (bootstrap)
Doctest: bootstrap.format_build_time ... ok
test_stage0_data (__main__.Stage0DataTestCase)
Extract data from stage0.txt ... ok
test_invalid_file (__main__.VerifyTestCase)
Should verify that the file is invalid ... invalid checksum:
    found:    334d016f755cd6dc58c53a86e183882f8ec14f52fb05345887c8a5edd42c87b7
    expected: 64ec88ca00b268e5ba1a35678a1b5316d212f4f366b2477232534a8aeca37f3c
ok
test_valid_file (__main__.VerifyTestCase)
Check if the sha256 sum of the given file is valid ... ok

----------------------------------------------------------------------
Ran 11 tests in 0.013s

OK
py35 create: /Development/rust/rust/src/bootstrap/.tox/py35
py35 inst: /Development/rust/rust/src/bootstrap/.tox/dist/rust-bootstrap-0.0.1.zip
py35 installed: rust-bootstrap==0.0.1
py35 runtests: PYTHONHASHSEED='3410238989'
py35 runtests: commands[0] | /Development/rust/rust/src/bootstrap/.tox/py35/bin/python bootstrap_test.py
bin_root (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bin_root ... ok
bootstrap_binary (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bootstrap_binary ... ok
cargo_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.cargo_stamp ... ok
get_mk (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_mk ... ok
get_string (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_string ... ok
get_toml (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_toml ... ok
rustc_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.rustc_stamp ... ok
format_build_time (bootstrap)
Doctest: bootstrap.format_build_time ... ok
test_stage0_data (__main__.Stage0DataTestCase)
Extract data from stage0.txt ... ok
test_invalid_file (__main__.VerifyTestCase)
Should verify that the file is invalid ... invalid checksum:
    found:    334d016f755cd6dc58c53a86e183882f8ec14f52fb05345887c8a5edd42c87b7
    expected: 64ec88ca00b268e5ba1a35678a1b5316d212f4f366b2477232534a8aeca37f3c
ok
test_valid_file (__main__.VerifyTestCase)
Check if the sha256 sum of the given file is valid ... ok

----------------------------------------------------------------------
Ran 11 tests in 0.021s

OK
py36 create: /Development/rust/rust/src/bootstrap/.tox/py36
py36 inst: /Development/rust/rust/src/bootstrap/.tox/dist/rust-bootstrap-0.0.1.zip
py36 installed: rust-bootstrap==0.0.1
py36 runtests: PYTHONHASHSEED='3410238989'
py36 runtests: commands[0] | /Development/rust/rust/src/bootstrap/.tox/py36/bin/python bootstrap_test.py
bin_root (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bin_root ... ok
bootstrap_binary (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.bootstrap_binary ... ok
cargo_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.cargo_stamp ... ok
get_mk (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_mk ... ok
get_string (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_string ... ok
get_toml (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.get_toml ... ok
rustc_stamp (bootstrap.RustBuild)
Doctest: bootstrap.RustBuild.rustc_stamp ... ok
format_build_time (bootstrap)
Doctest: bootstrap.format_build_time ... ok
test_stage0_data (__main__.Stage0DataTestCase)
Extract data from stage0.txt ... ok
test_invalid_file (__main__.VerifyTestCase)
Should verify that the file is invalid ... invalid checksum:
    found:    334d016f755cd6dc58c53a86e183882f8ec14f52fb05345887c8a5edd42c87b7
    expected: 64ec88ca00b268e5ba1a35678a1b5316d212f4f366b2477232534a8aeca37f3c
ok
test_valid_file (__main__.VerifyTestCase)
Check if the sha256 sum of the given file is valid ... ok

----------------------------------------------------------------------
Ran 11 tests in 0.019s

OK
___________________________________ summary ____________________________________
  py27: commands succeeded
  py35: commands succeeded
  py36: commands succeeded
  congratulations :)

aidanhs · 2017-07-05T17:32:53Z

@milmazz thanks for the PR! We'll make sure @alexcrichton or another reviewer gets to this soon.

One question - as far as I remember we try and keep this compatible with python 3, can you clarify what was broken?

milmazz · 2017-07-05T19:06:02Z

@aidanhs For example:

C:\Development\rust\src\bootstrap>python -V
Python 3.6.1

C:\Development\rust\src\bootstrap>python bootstrap_test.py
Traceback (most recent call last):
  File "bootstrap_test.py", line 21, in <module>
    import bootstrap
  File "C:\Users\tusc0958\Downloads\Development\rust\src\bootstrap\bootstrap.py", line 273
    except OSError, reason:
                  ^
SyntaxError: invalid syntax

Also, the README.md states that you need python 2.7 (but not 3.x) in the Building from Source section. Should I need to update that part? wdyt?

alexcrichton

Thanks! This part of the codebase very rarely regresses so the need to add tests isn't too high, but if you're willing go to through the hoops to add them then I don't see why we shouldn't land them. Can you ensure though that they run as part of our CI?

Otherwise python 3 compatibility is fine, but unfortuantely (last we checked) building LLVM required 2.7 (doesn't work with 3). That being said we can always be compatible for when they're compatible and then we can also provide nicer warnings when 3 is detected.

alexcrichton · 2017-07-05T19:56:16Z

src/bootstrap/bootstrap.py


    def download_stage0(self):
+        """Download stage0"""


Hm I personally prefer to avoid comments like this, it seems like this doesn't provide any utility over the name of the method itself?

@alexcrichton Thanks for the review! I agree with you, I added more information in both methods as you suggested. Please let me know what do you think.

alexcrichton · 2017-07-05T19:57:13Z

src/bootstrap/bootstrap.py

-        # If we're on NixOS we need to change the path to the dynamic loader
+    @staticmethod
+    def fix_executable(fname):
+        """This method tries to fix the given executable


Similarly as above, this doc string isn't too helpful :(

What's being "fixed" here?

aidanhs · 2017-07-05T22:26:40Z

I think this PR should also mean that users with a system version of LLVM to link against will be able to build without python 2 installed, which is something (not that system versions of LLVM are recommended).

aidanhs · 2017-07-05T22:30:56Z

src/bootstrap/bootstrap.py

-    return data
+        lines = [line.rstrip() for line in nightlies
+                 if not line.startswith("#")]
+        return dict([line.split(": ", 1) for line in lines if line])


Could you move the if line into the filtering above (if line and not line.startswith("#"))? Seems like it'd make more sense there.

@aidanhs The way you propose it will break the code when we try to split an empty line. First, we need to delete all the trailing whitespace (eg: \t\n\x0b\x0c\r ) with rstrip(), then we discard all the empty lines, after that, we can apply the split() method. HTH

Ah yes, I see.

aidanhs · 2017-07-05T22:37:17Z

src/bootstrap/bootstrap.py

            return
+        except OSError as reason:
+            if getattr(reason, 'winerror', None) is not None:
+                return


Surely this needs to raise in an else?

@aidanhs I tried to keep the same behavior, please note that we only catch subprocess.CalledProcessError, and WindowsError (which only occurs on Windows and it's a subclass of OSError). So, if any OSError exception occurs in another OS it will certainly raise an exception as you pointed out.

WDYT? Do we need to handle this case in other OSs?

I'd follow your current theme and leave any behaviour changes for a separate PR, i.e. restore the behaviour of throwing the exception upwards.

@milmazz do you have thoughts on the re-raise here?

@alexcrichton I think is not required to re-raise here because if the following condition is true:

if getattr(reason, 'winerror', None) is not None:

Then, we can agree that we don't need to modify the interpreter section to fix the dynamic linker in the given executable. This condition only applies on NixOS. wdyt?

Actually, after thinking a little bit more, you're right, we should have something like this:

except OSError as reason: if getattr(reason, 'winerror', None) is not None: return raise reason

If we receive an OSError exception that means that the system cannot find the
uname command and that is OK in Windows but not in other Operating Systems.

aidanhs · 2017-07-05T22:45:01Z

src/bootstrap/bootstrap.py

-        except subprocess.CalledProcessError as e:
-            print("warning: failed to call patchelf: %s" % e)
+        except subprocess.CalledProcessError as reason:
+            print("warning: failed to call patchelf:", reason)


You removed the %s on this print, but not the others - I don't think it matters, but it's probably worth being consistent.

@aidanhs Sure thing, I can change those lines too.

milmazz · 2017-07-07T20:47:39Z

@aidanhs @alexcrichton I think this is ready for another review. Please let me know what do you think.

In the meantime, I'll start learning some Rust :-)

aidanhs · 2017-07-09T18:47:59Z

appveyor.yml

@@ -125,6 +125,9 @@ install:
  - copy C:\Python27\python.exe C:\Python27\python2.7.exe
  - set PATH=C:\Python27;%PATH%

+  # Run unit tests for bootstrap.py
+  - python src\bootstrap\bootstrap_test.py


Why is this being run during the install phase?

@aidanhs I just thought it was a good place because the 2 preconditions that I need are already meet at this stage:

The repository is already cloned

Python is already configured

But I'm open to suggestions, please let me know where do you think is the right place to move this section.

Wherever it happens, it should probably be in the same place in both travis and appveyor. My personal preference it to put it in the "test" or "script" or whatever phase just for clarity.

In .travis.yml we can run custom commands before the script phase with before_script, it seems that the equivalent phase in appveyor.yml is before_build.

carols10cents · 2017-07-17T12:32:47Z

Ping @aidanhs, looks like this is back in your court for rereview?

aidanhs · 2017-07-18T00:03:24Z

appveyor.yml

+before_build:
+  # Run unit tests for bootstrap.py
+  - python src\bootstrap\bootstrap_test.py
+


Ok I've had a look at this again and realised I missed something - we run Docker containers for a number of our builds and so this python script should ideally run in those as well.

My suggestion is to remove the additions to the travis and appveyor yml, and instead mimic https://github.com/rust-lang/rust/blob/3447aa7/src/ci/run.sh#L65-L69 - hopefully that will work!

aidanhs · 2017-07-18T00:07:30Z

Other than the comment about moving this testing into run.sh, I think that's it from me! Let me know once you've had a chance to change that and we can see if the PR build works.

milmazz · 2017-07-18T22:03:51Z

@aidanhs Any idea why the build is failing now? is it related with the latest change?

Mark-Simulacrum · 2017-07-18T22:08:11Z

Build is actually green, just the mac builders were apparently canceled for some reason.

milmazz · 2017-07-18T22:25:50Z

@Mark-Simulacrum Yes, I saw it and I was curious about that. Anyway, I see that a new job was triggered for those cases. Thanks!

milmazz · 2017-07-18T22:28:20Z

Wow, that was fast! now the build is green :)

@aidanhs @alexcrichton Are we ready to go?

aidanhs · 2017-07-18T22:37:23Z

src/ci/run.sh

@@ -74,6 +74,12 @@ retry make prepare
 travis_fold end make-prepare
 travis_time_finish

+travis_fold start configure


Can you change the travis folds to be something like bootstrap-test? Both the start and the end need altering.

@aidanhs Good Catch! already fixed! 👍

aidanhs · 2017-07-19T13:29:37Z

Looking good! If you can squash your commits then I'll r+.

milmazz · 2017-07-19T14:01:07Z

@aidanhs Done!

Mark-Simulacrum · 2017-07-19T14:18:04Z

@bors r=aidanhs

bors · 2017-07-19T14:18:05Z

📌 Commit 72d4856 has been approved by aidanhs

bors · 2017-07-19T20:12:54Z

🔒 Merge conflict

milmazz · 2017-07-20T17:47:50Z

@aidanhs Could you please check why the build is failing? nothing appears in the raw log, I did a rebase to avoid the merge conflict with this line in the bootstrap.py file.

aidanhs · 2017-07-21T14:15:43Z

The failures were because the OSX builds didn't start for some reason. They're no-ops anyway, but I restarted them to get the PR build clean.

@bors r+

bors · 2017-07-21T14:15:44Z

📌 Commit 22f36db has been approved by aidanhs

@alexcrichton

bootstrap: Add doctests and unitests This commit includes the following changes: * Include more docstrings in classes, methods, and functions * Add doctests, which are great for self-documenting our source code * Add some unit tests with the `unittest` module * Remove `WindowsError` reference on non-windows systems * Rename some variables to be more explicit about their meaning * Move all the attributes defined outside of `__init__` * Add initial support for Python 3 r? @alexcrichton

Mark-Simulacrum · 2017-07-25T00:49:48Z

[00:02:06] /checkout/src/ci/run.sh: line 79: python: command not found -- I suspect something isn't using the configure script's output to find python...

Mark-Simulacrum · 2017-07-25T00:51:59Z

src/ci/run.sh

@@ -74,6 +74,12 @@ retry make prepare
 travis_fold end make-prepare
 travis_time_finish

+travis_fold start bootstrap-test
+travis_time_start
+python $SRC/src/bootstrap/bootstrap_test.py


Let's add this as a target to the makefile. I think you can edit Makefile.in to do so, and then we can use the CFG_PYTHON and CFG_SRC_DIR variables. check-bootstrap seems fine.

aidanhs · 2017-07-25T00:54:56Z

Wait, I don't understand why bors tested this again? I requested a change to put this in the makefile three days ago, this PR was assigned to me two days ago...and then bors tested it an hour ago?

Mark-Simulacrum · 2017-07-25T00:56:55Z

Huh. Possibly because we synchronized bors' queue and it somehow forgot about the failure, but odd indeed.

milmazz · 2017-07-25T05:30:04Z

@aidanhs @Mark-Simulacrum Done! 🤞

aidanhs · 2017-07-25T09:16:32Z

Thanks!

@bors r+

bors · 2017-07-25T09:16:33Z

📌 Commit 7c737b3 has been approved by aidanhs

bors · 2017-07-25T10:39:00Z

🔒 Merge conflict

bors · 2017-07-25T10:41:40Z

☔ The latest upstream changes (presumably #43384) made this pull request unmergeable. Please resolve the merge conflicts.

This commit includes the following: * Fix syntax errors in Python 3 * Include more docstrings in classes, methods, and functions * Include unit tests using `unittest` * Merge implementation of `{rustc,cargo}_out_of_date` * Merge implementation of `RustBuild.{cargo,rustc}` * Remove unnecessary source code * Move all the attributes defined outside of `__init__` * Remove remaining `%s` from print function * Remove `WindowsError` reference on non-windows systems * Rename some variables to be more explicit avoid their meaning * Run bootstrap tests in the CI process * Remove non-pythonic getters * Remove duplicate code in `download_stage0` method * Reduce the number of branches in `build_bootstrap` method * Re-raise exception when we cannot execute uname in non-windows systems * Avoid long lines

aidanhs · 2017-07-25T11:53:55Z

@bors r+

bors · 2017-07-25T11:53:55Z

📌 Commit f516765 has been approved by aidanhs

bors · 2017-07-25T12:06:35Z

⌛ Testing commit f516765 with merge 96acd85c692a74fb27aa8bbf1c39c86450b3c754...

bors · 2017-07-25T13:36:11Z

💔 Test failed - status-travis

aidanhs · 2017-07-25T13:52:28Z

@bors retry

Spurious failure in RLS tests #43453

bors · 2017-07-25T20:28:05Z

⌛ Testing commit f516765 with merge 549fbb6...

@alexcrichton

bootstrap: Add doctests and unitests This commit includes the following changes: * Include more docstrings in classes, methods, and functions * Add doctests, which are great for self-documenting our source code * Add some unit tests with the `unittest` module * Remove `WindowsError` reference on non-windows systems * Rename some variables to be more explicit about their meaning * Move all the attributes defined outside of `__init__` * Add initial support for Python 3 r? @alexcrichton

bors · 2017-07-25T22:06:12Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-07-25T22:09:53Z

@bors retry

RLS spurious failure - Spurious failure in RLS tests #43453

01:32:40] test test::test_borrow_error ... FAILED
[01:32:40] 
[01:32:40] failures:
[01:32:40] 
[01:32:40] ---- test::test_borrow_error stdout ----
[01:32:40] 	thread 'test::test_borrow_error' panicked at 'Hit timeout', /checkout/src/tools/rls/src/test/harness.rs:142:12
[01:32:40]

bors · 2017-07-25T23:46:19Z

⌛ Testing commit f516765 with merge b5e8a8e...

@alexcrichton

bootstrap: Add doctests and unitests This commit includes the following changes: * Include more docstrings in classes, methods, and functions * Add doctests, which are great for self-documenting our source code * Add some unit tests with the `unittest` module * Remove `WindowsError` reference on non-windows systems * Rename some variables to be more explicit about their meaning * Move all the attributes defined outside of `__init__` * Add initial support for Python 3 r? @alexcrichton

bors · 2017-07-26T02:12:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: aidanhs
Pushing b5e8a8e to master...

milmazz · 2017-07-26T13:39:48Z

@aidanhs @alexcrichton @Mark-Simulacrum Thank you for your help! 🎉 🕺 🎉

rust-highfive assigned brson Jul 4, 2017

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2017

alexcrichton reviewed Jul 5, 2017

View reviewed changes

aidanhs reviewed Jul 5, 2017

View reviewed changes

aidanhs reviewed Jul 9, 2017

View reviewed changes

aidanhs reviewed Jul 18, 2017

View reviewed changes

Mark-Simulacrum reviewed Jul 25, 2017

View reviewed changes

Mark-Simulacrum 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2017

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 Jul 25, 2017

bors merged commit f516765 into rust-lang:master Jul 26, 2017

milmazz deleted the bootstrap-unittest branch July 26, 2017 13:39

bootstrap: Add doctests and unitests #43046

bootstrap: Add doctests and unitests #43046

Conversation

milmazz commented Jul 4, 2017 • edited Loading

rust-highfive commented Jul 4, 2017

milmazz commented Jul 4, 2017

aidanhs commented Jul 5, 2017

milmazz commented Jul 5, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Jul 5, 2017

Choose a reason for hiding this comment

milmazz Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milmazz commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Jul 17, 2017

Choose a reason for hiding this comment

aidanhs commented Jul 18, 2017

milmazz commented Jul 18, 2017

Mark-Simulacrum commented Jul 18, 2017

milmazz commented Jul 18, 2017

milmazz commented Jul 18, 2017

aidanhs Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aidanhs commented Jul 19, 2017

milmazz commented Jul 19, 2017

Mark-Simulacrum commented Jul 19, 2017

bors commented Jul 19, 2017

bors commented Jul 19, 2017

milmazz commented Jul 20, 2017

aidanhs commented Jul 21, 2017

bors commented Jul 21, 2017

Mark-Simulacrum commented Jul 25, 2017

Choose a reason for hiding this comment

aidanhs commented Jul 25, 2017

Mark-Simulacrum commented Jul 25, 2017

milmazz commented Jul 25, 2017

aidanhs commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 25, 2017

aidanhs commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 25, 2017

aidanhs commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 25, 2017

Mark-Simulacrum commented Jul 25, 2017

bors commented Jul 25, 2017

bors commented Jul 26, 2017

milmazz commented Jul 26, 2017

milmazz commented Jul 4, 2017 •

edited

Loading

milmazz Jul 6, 2017 •

edited

Loading

aidanhs Jul 18, 2017 •

edited

Loading