Skip to content

/ servo

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

Fix nested modules while imported under more than 3 levels #27034

Merged
merged 2 commits into from Jun 23, 2020
Merged

Fix nested modules while imported under more than 3 levels #27034

merged 2 commits into from Jun 23, 2020

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented Jun 22, 2020

This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #27029
  • There are tests for these changes
@highfive
Copy link

highfive commented Jun 22, 2020

Heads up! This PR modifies the following files:
@servo-wpt-sync servo-wpt-sync mentioned this pull request Jun 22, 2020
Merged
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 22, 2020

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24284.
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 22, 2020

r? @jdm or @Manishearth
@highfive highfive assigned jdm and unassigned ferjm Jun 22, 2020
@jdm jdm assigned Manishearth and unassigned jdm Jun 22, 2020
@CYBAI CYBAI mentioned this pull request Jun 22, 2020
Closed
@Manishearth
Manishearth approved these changes Jun 22, 2020
View changes
Copy link
Member

Manishearth left a comment •
edited

Could you mention the commit range being reverted and PR in the first commit?
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 22, 2020

Could you mention the commit range being reverted and PR in the first commit?

Sorry, I don't actually revert a commit because it was squashed in the #26395; maybe it's good to just mention the PR?
@CYBAI CYBAI force-pushed the CYBAI:fix-nested-modules branch from 7d61719 to 5e97746 Jun 22, 2020
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 22, 2020

Hope the first new commit is clear enough 🙏
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 22, 2020

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24284.
@Manishearth
Copy link
Member

Manishearth commented Jun 22, 2020

@CYBAI right, hence the mention of the commit range
CYBAI added 2 commits Jun 22, 2020
@CYBAI
Rollback to recursively check dependency status
a7221fd 
In https://github.com/servo/servo/pull/26395/files#diff-3fe97584f564214ec8e7ebbf91747e03L253-R318,
we moved from `recursive checking` of dependency status to check only the
_current module_'s dependency status and its descendant dependency status and
also circular dependency status.

However, it will cause an issue.

For example, if the module dependency is like following

```
a -> b -> c -> d -> e
f -> g -> h -> c -> d -> e
```

In this example, if the d module is still under fetching but g is trying
to advance to finish. Then, it will cause a panic because module d is
g's grand-grand-grand-descendant which means it's still under fetching
and we can't instantiate module g.

Ideally, we should get rid of the checking in #26903 so, before #26903
fixed, we can just move back to the recursive checking way which will
ensure all descendants are not fetching.
@CYBAI
Test imports under more than 3 levels in different modules
Loading status checks…
0c938b3
@CYBAI CYBAI force-pushed the CYBAI:fix-nested-modules branch from 5e97746 to 0c938b3 Jun 23, 2020
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 23, 2020

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#24284.
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 23, 2020

@Manishearth I'm still not so clear what you mean commit range here 🤔 but I just updated the commit message to include the changes of the line number in the PR (https://github.com/servo/servo/pull/26395/files#diff-3fe97584f564214ec8e7ebbf91747e03L253-R318).
@Manishearth
Copy link
Member

Manishearth commented Jun 23, 2020

never mind, this is fine

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

📌 Commit 0c938b3 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

⌛ Testing commit 0c938b3 with merge b4d13b8...
bors-servo added a commit that referenced this pull request Jun 23, 2020
@bors-servo
Auto merge of #27034 - CYBAI:fix-nested-modules, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
b4d13b8 
Fix nested modules while imported under more than 3 levels

This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27029
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

💔 Test failed - status-taskcluster
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 23, 2020

@bors-servo retry
bors-servo added a commit that referenced this pull request Jun 23, 2020
@bors-servo
Auto merge of #27034 - CYBAI:fix-nested-modules, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6e89817 
Fix nested modules while imported under more than 3 levels

This is kind of workaround to fix the issue but #26903 should provide much better solution to remove the checking.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27029
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

⌛ Testing commit 0c938b3 with merge 6e89817...
@jdm
Copy link
Member

jdm commented Jun 23, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

⌛ Testing commit 0c938b3 with merge c76d131...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 23, 2020

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing c76d131 to master...
@bors-servo bors-servo merged commit c76d131 into servo:master Jun 23, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jun 23, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1592881946733.
@CYBAI CYBAI deleted the CYBAI:fix-nested-modules branch Jun 23, 2020
@CYBAI
Copy link
Collaborator Author

CYBAI commented Jun 23, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1592881946733.

It might be caused by the CI failure? I saw this in wpt-chrome-dev-results job but, IIRC, the CI pass when the upstream PR just created 🤔

Traceback (most recent call last):
  File "./wpt", line 21, in <module>
    wpt.main()
  File "/home/test/web-platform-tests/tools/wpt/wpt.py", line 171, in main
    rv = script(*args, **kwargs)
  File "/home/test/web-platform-tests/tools/wpt/run.py", line 803, in run
    **kwargs)
  File "/home/test/web-platform-tests/tools/wpt/run.py", line 773, in setup_wptrunner
    setup_cls.setup(kwargs)
  File "/home/test/web-platform-tests/tools/wpt/run.py", line 183, in setup
    self.setup_kwargs(kwargs)
  File "/home/test/web-platform-tests/tools/wpt/run.py", line 345, in setup_kwargs
    browser_binary=kwargs["binary"],
  File "/home/test/web-platform-tests/tools/wpt/browser.py", line 643, in install_webdriver
    self.version(browser_binary), dest)
  File "/home/test/web-platform-tests/tools/wpt/browser.py", line 631, in install_webdriver_by_version
    unzip(get(url).raw, dest)
  File "/home/test/web-platform-tests/tools/wpt/utils.py", line 78, in unzip
    fileobj = seekable(fileobj)
  File "/home/test/web-platform-tests/tools/wpt/utils.py", line 62, in seekable
    return BytesIO(fileobj.read())
  File "/home/test/web-platform-tests/_venv2/lib/python2.7/site-packages/urllib3/response.py", line 541, in read
    raise IncompleteRead(self._fp_bytes_read, self.length_remaining)
  File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/test/web-platform-tests/_venv2/lib/python2.7/site-packages/urllib3/response.py", line 455, in _error_catcher
    raise ProtocolError("Connection broken: %r" % e, e)
urllib3.exceptions.ProtocolError: ('Connection broken: IncompleteRead(4463202 bytes read, 2500735 more expected)', IncompleteRead(4463202 bytes read, 2500735 more expected))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Nested imports while importing higher than 2 level modules will panic
7 participants
@CYBAI @highfive @servo-wpt-sync @Manishearth @bors-servo @jdm @ferjm
You can’t perform that action at this time.