Skip to content

Provide more info about failuers to load CLR assemblies #1076

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 19, 2020
Merged

Provide more info about failuers to load CLR assemblies #1076

merged 1 commit into from
Aug 19, 2020

Conversation

lostmsu
Copy link
Member

@lostmsu lostmsu commented Mar 4, 2020

What does this implement/fix? Explain your changes.

  1. When trying to implicitly load assemblies, and that fails not because an assembly is missing, but because loading failed for some reason, emit Python warning.
  2. When trying to import a module in our import hook, if the module name is an assembly name, and CLR fails to load it, and Python also fails to find a module with the same name, add the exceptions we got from both during the attempt into __cause__ attribute of the final ImportError.

BREAKING: clr.AddReference will now throw exceptions besides FileNotFoundException.

Additional: a few uses of BorrowedReference

Does this close any currently open issues?

This addresses #261
It is an alternative to #298

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example

@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from 645687e to 4e1ea84 Compare March 4, 2020 02:47
@lostmsu lostmsu mentioned this pull request Mar 4, 2020
Closed
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch 2 times, most recently from ccf5203 to e6610eb Compare April 27, 2020 21:35
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from e6610eb to 46a51d4 Compare May 13, 2020 21:05
@codecov-io
Copy link

Codecov Report

Merging #1076 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1076   +/-   ##
=======================================
  Coverage   86.66%   86.66%           
=======================================
  Files           1        1           
  Lines         300      300           
=======================================
  Hits          260      260           
  Misses         40       40
Flag Coverage Δ
#setup_linux 65.33% <ø> (ø)
#setup_windows 72.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72fae73...46a51d4. Read the comment docs.

@lostmsu lostmsu added this to the 3.0.0 milestone May 14, 2020
filmor
filmor reviewed Aug 15, 2020
View reviewed changes
src/runtime/assemblymanager.cs Outdated
if (Path.IsPathRooted(name))
{
if (!Path.HasExtension(name))
if (!Path.HasExtension(name) && Runtime.InteropVersion < new Version(3, 0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was for compatibility, you can probably remove it.

@filmor
Copy link
Member

filmor commented Aug 15, 2020

Fine by me, but please add a changelog entry for the breaking change.

@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch 4 times, most recently from 58ef45f to 37df647 Compare August 18, 2020 20:43
@lostmsu
do not eat all exceptions when trying to add CLR reference, provide m…
6eb9258 
…ore info when CLR assemblies are failed to be loaded

1. When trying to implicitly load assemblies, and that fails NOT because an assembly is missing, but because loading failed for some reason, emit Python warning.
2. When trying to import a module in our import hook, if the module name is an assembly name, and we fail to load it, and Python also fails to find a module with the same name, add the exceptions we got during the attempt to load it into __cause__ of the final ImportError

BREAKING: clr.AddReference will now throw exceptions besides FileNotFoundException.

Additional: a few uses of BorrowedReference

This addresses pythonnet#261
It is an alternative to pythonnet#298
@lostmsu lostmsu force-pushed the PR/ImportErrorImprovement branch from 37df647 to 6eb9258 Compare August 18, 2020 21:14
@lostmsu
Copy link
Member Author

lostmsu commented Aug 18, 2020

I dropped the update, that tried to remove legacy "CLR." module from this change, as it started growing in scope, and is not directly related to this change.

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2020 •
edited
Loading

Codecov Report

Merging #1076 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1076   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          251      251           
  Misses         40       40
Flag Coverage Δ
#setup_linux 64.94% <ø> (ø)
#setup_windows 72.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e1fc2e...6eb9258. Read the comment docs.

@lostmsu lostmsu merged commit 5b989cb into pythonnet:master Aug 19, 2020
@lostmsu lostmsu deleted the PR/ImportErrorImprovement branch August 19, 2020 06:29
@filmor filmor mentioned this pull request Feb 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmor filmor filmor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lostmsu @codecov-io @filmor @codecov-commenter