NotificationsYou must be signed in to change notification settings
Fork752
Star5.1k

Add pysetargv#347

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vmuriart merged 5 commits intopythonnet:masterfromfilmor:add-pysetargv

Feb 1, 2017

Merged

Add pysetargv#347

vmuriart merged 5 commits intopythonnet:masterfromfilmor:add-pysetargv

Feb 1, 2017

Conversation

Copy link

Member

filmor commentedJan 31, 2017•
edited
Loading

What does this implement/fix? Explain your changes.

Implements an overload ofInitialize and aPy.SetArgv function that allow the user to issue aPySys_SetArgvEx call. By default, this call is done onInitialize with a list that contains a single empty string (['']), using the arguments supplied to the .NET process if available.

Does this close any currently open issues?

#299.

Any other comments?

Based on thefix-shutdown branch as otherwise I'm not able to run the unit-tests reliably..

Copy link

Contributor

vmuriart commentedJan 31, 2017

This replaces#301?

vmuriart reviewed

Jan 31, 2017

View reviewed changes

src/runtime/runtime.cs Outdated

		@@ -2027,11 +2027,26 @@ internal unsafe static extern IntPtr
		internal unsafe static extern IntPtr
		PyImport_GetModuleDict();


		#if !(PYTHON26 \|\| PYTHON27)

Copy link

Contributor

vmuriartJan 31, 2017

Choose a reason for hiding this comment

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

Can this be#if PYTHON3 ?

Copy link

MemberAuthor

filmorJan 31, 2017

Choose a reason for hiding this comment

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

Yes, I'll change it.

Copy link

MemberAuthor

filmor commentedJan 31, 2017

Yes, it's an alternative using directly the Python API function.

den-run-ai reviewed

Jan 31, 2017

View reviewed changes

src/embed_tests/InitializeTest.cs

		using (new PythonEngine())
		using (var argv = new PyList(Runtime.Runtime.PySys_GetObject("argv")))
		{
		Assert.That(argv.Length() != 0);

Copy link

Contributor

den-run-aiJan 31, 2017

Choose a reason for hiding this comment

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

first need to check that it is not null

Copy link

MemberAuthor

filmorJan 31, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Why? If it isnull, this will fail with aNullReferenceException, perfectly fine. This PR is supposed to ensure thatsys.argv is always initialised.

den-run-ai reviewed

Jan 31, 2017

View reviewed changes

src/runtime/Python.Runtime.csproj Outdated

		@@ -92,6 +92,7 @@
		<Optimize>false</Optimize>
		<DebugType>full</DebugType>
		<PlatformTarget>x64</PlatformTarget>
		<DefineConstants>TRACE;DEBUG;PYTHON3;PYTHON35;UCS2</DefineConstants>

Copy link

Contributor

den-run-aiJan 31, 2017

Choose a reason for hiding this comment

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

this gets overwritten by setup.py or specific needs of the user, so no need to update this

Copy link

Contributor

vmuriartJan 31, 2017

Choose a reason for hiding this comment

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

He probably added that during his development and forgot to remove it.

Copy link

Contributor

vmuriartJan 31, 2017

Choose a reason for hiding this comment

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

Updated#346 to include configuration for PY3 development.

Copy link

MemberAuthor

filmorJan 31, 2017

Choose a reason for hiding this comment

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

Yep, exactly that, I'll remove this line.

Copy link

Contributor

den-run-ai commentedJan 31, 2017

I'm OK with merging this version as long as comments are addressed

filmor force-pushed theadd-pysetargv branch fromebbdb35 to174e1faCompare

January 31, 2017 21:17

filmor added5 commits

January 31, 2017 15:08

Implement Py.SetArgv.

eca5ac9

Add aPy.Throw function.

331524f

Checks whether an error occurred and in case it has throws aPythonException.

Add overload for Initialize to pass specific args.

a0849a9

Allow the engine to be initialized as a Disposable.

4f67c5d

Use Py.Throw in a few more places.

3785c40

In particular in Py.Import.

Copy link

Contributor

vmuriart commentedJan 31, 2017

@filmor rebased your work here

https://github.com/pythonnet/pythonnet/tree/pysetargv

vmuriart force-pushed theadd-pysetargv branch from174e1fa to3785c40Compare

February 1, 2017 01:42

Copy link

codecov-io commentedFeb 1, 2017•
edited
Loading

Codecov Report

Merging#347 intomaster willincrease coverage by-0.08%.

@@            Coverage Diff             @@##           master     #347      +/-   ##==========================================- Coverage   61.45%   61.37%   -0.08%==========================================  Files          61       61                Lines        5336     5369      +33       Branches      900      896       -4     ==========================================+ Hits         3279     3295      +16- Misses       1819     1853      +34+ Partials      238      221      -17

Impacted Files	Coverage Δ
src/runtime/runtime.cs	`81.32% <ø> (ø)`	✅
src/runtime/pythonengine.cs	`54.16% <36.36%> (-1.18%)`	❌
src/runtime/managedtype.cs	`46.15% <ø> (ø)`	✅
src/runtime/delegatemanager.cs	`87.6% <ø> (ø)`	✅
src/runtime/converter.cs	`75.38% <ø> (ø)`	✅
src/runtime/interfaceobject.cs	`43.75% <ø> (ø)`	✅
src/runtime/classderived.cs	`0% <ø> (ø)`	✅
src/runtime/pyobject.cs	`22.22% <ø> (ø)`	✅
src/runtime/exceptions.cs	`72.8% <ø> (ø)`	✅

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatef7f2fc0...3785c40. Read thecomment docs.

vmuriart merged commit3785c40 intopythonnet:master

Feb 1, 2017

vmuriart added a commit that referenced this pull request

Feb 1, 2017

Merge pull request#347from filmor/add-pysetargv

7cab332

Closes#347

filmor mentioned this pull request

Feb 1, 2017

Ensure that sys.argv is set#301

Merged

Copy link

Contributor

den-run-ai commentedFeb 1, 2017

@filmor the coverage on pythonengine.cs reduced

Copy link

Contributor

vmuriart commentedFeb 1, 2017

It's probably the refactoring he did. The coverage needs some fine tuning, its definetly not as robust as python's coverage engine.

den-run-ai mentioned this pull request

Feb 24, 2017

Compatibility issue between argparse and pythonnet packages#404

Closed

den-run-ai reviewed

Feb 24, 2017

View reviewed changes

src/runtime/interop.cs Outdated

		@@ -227,7 +227,7 @@ static ModuleDefOffset()
		byte[] ascii = Encoding.ASCII.GetBytes(modulename);
		int size = name + ascii.Length + 1;
		IntPtr ptr = Marshal.AllocHGlobal(size);
		for (int i = 0; i <= m_free; i += IntPtr.Size)
		for (int i = 0; i < m_free; i += IntPtr.Size)

Copy link

Contributor

den-run-aiJan 31, 2017

Choose a reason for hiding this comment

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

where m_free is set?

src/runtime/pythonengine.cs


		namespace Python.Runtime
		{
		/// <summary>
		/// This class provides the public interface of the Python runtime.
		/// </summary>
		public class PythonEngine
		public class PythonEngine : IDisposable

Copy link

Contributor

den-run-aiJan 31, 2017

Choose a reason for hiding this comment

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

why is this now IDisposable?

Copy link

MemberAuthor

filmorFeb 24, 2017

Choose a reason for hiding this comment

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

Because that's easier to use correctly thanInitialize andShutdown.

src/runtime/pythonengine.cs

		{
		throw new PythonException();
		}
		Py.Throw();

Copy link

Contributor

den-run-aiJan 31, 2017

Choose a reason for hiding this comment

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

why not also check for IntPtr.Zero?

Copy link

MemberAuthor

filmorFeb 24, 2017

Choose a reason for hiding this comment

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

Because this function either returns null and sets an exception, or it returns a module.

src/runtime/pythonengine.cs

		{
		Initialize(Enumerable.Empty<string>());
		}

Copy link

Contributor

den-run-aiFeb 8, 2017

Choose a reason for hiding this comment

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

don't we need to check if sys.argv is non-empty in the case of extending withimport clr?@filmor @vmuriart anyway let's see if this passes my tests

Copy link

Contributor

den-run-aiFeb 24, 2017

Choose a reason for hiding this comment

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

i did not click submit review few weeks ago

filmor deleted the add-pysetargv branch

March 6, 2019 16:20

Labels

None yet

4 participants

Movatterモバイル変換

Add pysetargv#347

Add pysetargv#347

Uh oh!

Conversation

filmor commentedJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Uh oh!

vmuriart commentedJan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commentedJan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmorJan 31, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

den-run-ai commentedJan 31, 2017

Uh oh!

vmuriart commentedJan 31, 2017

Uh oh!

codecov-io commentedFeb 1, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

den-run-ai commentedFeb 1, 2017

Uh oh!

vmuriart commentedFeb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

filmor commentedJan 31, 2017•
edited
Loading

filmorJan 31, 2017•
edited
Loading

codecov-io commentedFeb 1, 2017•
edited
Loading