Use the improved submodule handling #42081

ishitatsuyuki · 2017-05-18T08:35:43Z

That was a crap...

Updating submodules
Traceback (most recent call last):
  File "./x.py", line 20, in <module>
    bootstrap.main()
  File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 684, in main
    bootstrap()
  File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 662, in bootstrap
    rb.update_submodules()
  File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 566, in update_submodules
    path = line[1:].split(' ')[1]
TypeError: a bytes-like object is required, not 'str'

Maybe we need to confirm the compatibility of git options, such as git config or git -C (I believe they existed long before, though). This is tested locally.

ghost · 2017-05-18T11:00:59Z

Just a quick feedback on that: git -C isn't that old, RHEL6 doesn't handle it for example.

ishitatsuyuki · 2017-05-18T11:06:07Z

If possible, can you also check for any Python 2 incompatibilities?

alexcrichton · 2017-05-18T16:06:51Z

r? @aidanhs

Mind taking a look at this? I think you know more about submodules than I

kennytm · 2017-05-19T05:52:49Z

@ishitatsuyuki The Python 2/3 thing is the same as #42085? (fixed in #42089)

ishitatsuyuki · 2017-05-19T11:29:34Z

I have rebased the things. This should supersede #42089, if it was possible to gain approval.

... sorry for the redundant formatting, I couldn't keep up without my auto formatter.

aidanhs · 2017-05-19T17:23:14Z

src/bootstrap/bootstrap.py

+        # submodule path
+        submodules = [s.split()[1] for s in subprocess.check_output(
+            ["git", "config", "--file", os.path.join(
+                self.rust_root, ".gitmodules"), "--get-regexp", "path"]).splitlines()]


Unfortunately we probably do actually need to do the defaultencoding dance even here, doesn't windows sometimes use two-byte unicode encodings? Which would mean checking for b"llvm" wouldn't work.

I don't think so. The command line uses the current code page, which IIRC is ASCII compat. Minding trying out that?

You are indeed correct.

That said, I feel like being consistent with the rest of the file (which uses defaultencoding) is probably worthwhile. Indeed, two reviewers of this PR have already been confused by imagined possible problems!

bors · 2017-05-19T23:40:26Z

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

ishitatsuyuki · 2017-05-20T05:08:41Z

Rebase made with the theirs strategy option.

Mark-Simulacrum · 2017-05-20T12:17:29Z

src/bootstrap/bootstrap.py

+        submodules = [module for module in submodules
+                      if not ((module.endswith(b"llvm") and
+                              (self.get_toml('llvm-config') or self.get_mk('CFG_LLVM_ROOT'))) or
+                      (module.endswith(b"jemalloc") and


I'm fairly certain that using b"..." will break Python 3 compatibility, see #42089.

... what if I tell you that I happily ran it on my Arch laptop which has Py3?

Hm, odd. Not sure why it was changed in that PR then. Never mind me.

It seems that the original PR decoded the pipe output which is in system default encoding, and can vary on Windows. However, I'm quite sure that all code pages are ASCII compat, which doesn't pose a problem if you're dealing with pure English paths. (want some emoji there? :P)

The original PR took didn't decode at all, which was the problem - it took a bytestring of submodules and tried split it with a string, which isn't allowed. The followup PR made sure the bytestring was converted to a string like all the other outputs from commands in the file, so now using bytestrings would be incorrect. This PR goes back to not decoding command output, so bytestrings are correct again.

aidanhs · 2017-05-20T16:20:09Z

Tested on Centos 6, r=me after moving from bytestrings to defaultencoding for consistency (if you happen to undo the autoformat diff noise at the same time then that'd also be fantastic).

ishitatsuyuki · 2017-05-21T01:32:55Z

@aidanhs I'm not sure if getdefaultencoding() is a good idea. It's almost constant ('ascii' for 2, 'utf8' for 3), and isn't so useful for Windows.

aidanhs · 2017-05-22T01:44:06Z

My logic is simply that it's ideal if this file is consistent. Since the rest of the file uses getdefaultencoding at all entry points of text into the script and then works exclusively with unicode strings, why not do the same here? The issue with the previous PR was caused by a mixing of byte and unicode strings - making sure everything is always unicode helps avoid potential problems.

ishitatsuyuki · 2017-05-22T07:09:54Z

Roughly tested on both Arch and CentOS 6.

aidanhs · 2017-05-22T18:17:15Z

Great! Thanks very much for bearing with me @ishitatsuyuki :)

@bors r+

bors · 2017-05-22T18:17:16Z

📌 Commit d34aaa1 has been approved by aidanhs

@alexcrichton

…idanhs Use the improved submodule handling r? @alexcrichton That was a crap... ``` Updating submodules Traceback (most recent call last): File "./x.py", line 20, in <module> bootstrap.main() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 684, in main bootstrap() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 662, in bootstrap rb.update_submodules() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 566, in update_submodules path = line[1:].split(' ')[1] TypeError: a bytes-like object is required, not 'str' ``` Maybe we need to confirm the compatibility of git options, such as `git config` or `git -C` (I believe they existed long before, though). This is tested locally.

@alexcrichton

…idanhs Use the improved submodule handling r? @alexcrichton That was a crap... ``` Updating submodules Traceback (most recent call last): File "./x.py", line 20, in <module> bootstrap.main() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 684, in main bootstrap() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 662, in bootstrap rb.update_submodules() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 566, in update_submodules path = line[1:].split(' ')[1] TypeError: a bytes-like object is required, not 'str' ``` Maybe we need to confirm the compatibility of git options, such as `git config` or `git -C` (I believe they existed long before, though). This is tested locally.

bors · 2017-05-25T04:40:11Z

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

Also, improve the split mechanism to address space in paths.

ishitatsuyuki · 2017-05-25T05:01:06Z

Rebased. Didn't tested on CentOS yet, but I believe it should be fine. Works fine on Arch with both Python 2/3.

ishitatsuyuki · 2017-05-26T04:30:16Z

Review please. Basically I think this is tested and reviewed enough and it's fine to r+ as I only did a rebase.

aidanhs · 2017-05-26T13:31:17Z

@bors r+

bors · 2017-05-26T13:31:18Z

📌 Commit 15c26c9 has been approved by aidanhs

bors · 2017-05-26T18:12:00Z

⌛ Testing commit 15c26c9 with merge 5579677...

@alexcrichton

Use the improved submodule handling r? @alexcrichton That was a crap... ``` Updating submodules Traceback (most recent call last): File "./x.py", line 20, in <module> bootstrap.main() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 684, in main bootstrap() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 662, in bootstrap rb.update_submodules() File "/home/ishitatsuyuki/Documents/rust/src/bootstrap/bootstrap.py", line 566, in update_submodules path = line[1:].split(' ')[1] TypeError: a bytes-like object is required, not 'str' ``` Maybe we need to confirm the compatibility of git options, such as `git config` or `git -C` (I believe they existed long before, though). This is tested locally.

bors · 2017-05-26T20:52:23Z

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

rust-highfive assigned alexcrichton May 18, 2017

rust-highfive assigned aidanhs and unassigned alexcrichton May 18, 2017

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2017

ishitatsuyuki force-pushed the submodule-better branch from 914f6da to 97e7f9c Compare May 19, 2017 11:27

aidanhs reviewed May 19, 2017

View reviewed changes

ishitatsuyuki force-pushed the submodule-better branch from b153b03 to 8f111f3 Compare May 20, 2017 05:08

Mark-Simulacrum reviewed May 20, 2017

View reviewed changes

frewsxcv mentioned this pull request May 23, 2017

Rollup of 5 pull requests #42173

Closed

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-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2017

frewsxcv mentioned this pull request May 24, 2017

Rollup of 8 pull requests #42185

Closed

ishitatsuyuki added 5 commits May 25, 2017 13:57

Use the improved submodule handling

d552e34

Format bootstrap.py using autopep8

1fea55b

bootstrap.py: Filter instead of iteration

5bcf06a

bootstrap.py: decode to str

46ebd83

Also, improve the split mechanism to address space in paths.

bootstrap.py: support verbose for submodules

15c26c9

ishitatsuyuki force-pushed the submodule-better branch from d34aaa1 to 15c26c9 Compare May 25, 2017 05:00

bors mentioned this pull request May 26, 2017

querify layout and move param env out of the infcx #42189

Merged

bors merged commit 15c26c9 into rust-lang:master May 26, 2017

ishitatsuyuki deleted the submodule-better branch July 15, 2018 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the improved submodule handling #42081

Use the improved submodule handling #42081

ishitatsuyuki commented May 18, 2017 •

edited

Loading

ghost commented May 18, 2017

ishitatsuyuki commented May 18, 2017

alexcrichton commented May 18, 2017

kennytm commented May 19, 2017

ishitatsuyuki commented May 19, 2017

aidanhs May 19, 2017

ishitatsuyuki May 19, 2017

aidanhs May 20, 2017

bors commented May 19, 2017

ishitatsuyuki commented May 20, 2017

Mark-Simulacrum May 20, 2017

ishitatsuyuki May 20, 2017

Mark-Simulacrum May 20, 2017

ishitatsuyuki May 20, 2017

aidanhs May 20, 2017

aidanhs commented May 20, 2017

ishitatsuyuki commented May 21, 2017

aidanhs commented May 22, 2017

ishitatsuyuki commented May 22, 2017

aidanhs commented May 22, 2017

bors commented May 22, 2017

bors commented May 25, 2017

ishitatsuyuki commented May 25, 2017

ishitatsuyuki commented May 26, 2017

aidanhs commented May 26, 2017

bors commented May 26, 2017

bors commented May 26, 2017

bors commented May 26, 2017

Use the improved submodule handling #42081

Use the improved submodule handling #42081

Conversation

ishitatsuyuki commented May 18, 2017 • edited Loading

ghost commented May 18, 2017

ishitatsuyuki commented May 18, 2017

alexcrichton commented May 18, 2017

kennytm commented May 19, 2017

ishitatsuyuki commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 19, 2017

ishitatsuyuki commented May 20, 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

Choose a reason for hiding this comment

aidanhs commented May 20, 2017

ishitatsuyuki commented May 21, 2017

aidanhs commented May 22, 2017

ishitatsuyuki commented May 22, 2017

aidanhs commented May 22, 2017

bors commented May 22, 2017

bors commented May 25, 2017

ishitatsuyuki commented May 25, 2017

ishitatsuyuki commented May 26, 2017

aidanhs commented May 26, 2017

bors commented May 26, 2017

bors commented May 26, 2017

bors commented May 26, 2017

ishitatsuyuki commented May 18, 2017 •

edited

Loading