Skip to content
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

DYN-4800: Bump Python version to 3.9.12 #12815

Merged
merged 8 commits into from
May 6, 2022
Merged

Conversation

sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Apr 20, 2022

Purpose

This pull request does:

  • Bump pythonnet to 2.5.2
  • Bump embedded python to 3.9.12

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@sm6srw sm6srw added the WIP label Apr 20, 2022
@sm6srw sm6srw requested a review from pinzart90 May 4, 2022 13:49
@sm6srw sm6srw removed the WIP label May 4, 2022
@sm6srw sm6srw requested review from pinzart and mjkkirschner May 4, 2022 13:49
@@ -376,6 +376,8 @@ public DSCPythonCodeCompletionProviderCore()
BasicVariableTypes.Add(Tuple.Create(LIST_VARIABLE, typeof(PyList)));
BasicVariableTypes.Add(Tuple.Create(DICT_VARIABLE, typeof(PyDict)));

Python.Included.Installer.SetupPython().Wait();
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary? The DSCPython engine should do this already, should it not?
I think it's worth testing this change in Revit context if it's really required.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a note in Dynamo

NOTE: Calling SetupPython multiple times will add the install location to the path many times,
        /// potentially causing the environment variable to overflow.

I think we might be doing this already ...maybe we should try to only run SetupPython() only once ...(keep a isAlreadySetup flag somewhere)
Please search for all occurrences of SetupPython()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually was able to get in here without having python initialized. And we are already calling this method in multiple places. I will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now calling SetupPython() every where. That method maintain a just do it once flag.

@@ -336,7 +336,7 @@ import os
/// NOTE: Calling SetupPython multiple times will add the install location to the path many times,
/// potentially causing the environment variable to overflow.
/// </summary>
private static void InstallPython()
internal static void InstallPython()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it internal. Would it be OK to make this public?

Copy link
Member

Choose a reason for hiding this comment

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

since it has a side effect that is potentially dangerous I'd prefer if extensions could not call it.

@sm6srw
Copy link
Contributor Author

sm6srw commented May 5, 2022

@mjkkirschner PTAL!

@sm6srw sm6srw added the PTAL Please Take A Look 👀 label May 5, 2022
@sm6srw sm6srw merged commit 2b74138 into DynamoDS:master May 6, 2022
@sm6srw sm6srw deleted the DYN-4802-2 branch May 6, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants