Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: fix problems with windows commands#942

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

Merged
codejedi365 merged 11 commits intopython-semantic-release:masterfromJCHacking:fix_windows_build_command
Jun 5, 2024
Merged

fix: fix problems with windows commands#942

codejedi365 merged 11 commits intopython-semantic-release:masterfromJCHacking:fix_windows_build_command
Jun 5, 2024

Conversation

@JCHacking
Copy link
Contributor

@JCHackingJCHacking commentedMay 30, 2024
edited
Loading

Depending on the command it may fail in windows because:

  • The SYSTEMROOT environment variable (refers to the windows installation path C:Windows) is missing.
  • The WINDIR environment variable (Same as SYSTEMROOT) is missing.

I think it makes sense to include it by default instead of adding it when they have problems because it is not an environment variable as it could be SLOW_TEST=True or something like that.

GabrielOctavianPopa reacted with thumbs up emoji
@JCHackingJCHacking changed the titlefix: Fix problems with windows commandsDRAFT fix: Fix problems with windows commandsMay 31, 2024
@JCHackingJCHacking changed the titleDRAFT fix: Fix problems with windows commandsfix: Fix problems with windows commandsMay 31, 2024
@JCHackingJCHacking changed the titlefix: Fix problems with windows commandsfix: fix problems with windows commandsMay 31, 2024
@codejedi365
Copy link
Contributor

@JCHacking please update the testtest_version_runs_build_command with your windows env var. We only want to validate that a set env var is passed to the build command. You do not need to add this to the following test of user defined env vars.

Please also document your newly passed env variable to thebuild_command table of all env variable definitions ondocs/configuration.rst. Please include a windows relevant clause and what would happen on non-windows systems.

@codejedi365
Copy link
Contributor

Also, I agree this is a good addition to adjust the default shell env for windows related env vars. Please be thorough as I am unaware of what scripts on windows would need.

@JCHacking
Copy link
ContributorAuthor

Hi@codejedi365
Added in the tests and in the documentation.
I will check if there are any other windows environment variables that deserve to be added.

In the case of non-windows systems, it is normal that none of these systems are present, so they will not be imported.
Would it be worth adding a validation of whether the platform is windows to add these? Or do we understand that the probability is very small?

@JCHacking
Copy link
ContributorAuthor

I have been looking at the poetry and pdm code to see what environment variables they use and get an idea. There is a lot of variety.

These are the variables documented by microsofthttps://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables

Do I add them all?

@codejedi365
Copy link
Contributor

codejedi365 commentedJun 1, 2024
edited
Loading

In the case of non-windows systems, it is normal that none of these systems are present, so they will not be imported.

Agreed; I was just recommending this be included in the docs.

Would it be worth adding a validation of whether the platform is windows to add these? Or do we understand that the probability is very small?

No, I don't think so. The current code uses a None Filter so if they don't exist they won't be passed through anyway. Even if they do exist it won't cause much difference. The reason I am selectively passing them is to ensure no secrets are accidentally passed on that are not explicitly set by the user inbuild_command_env.

Edit: My answer is now yes, as part of that windows function, as there are so many vars

@codejedi365
Copy link
Contributor

codejedi365 commentedJun 1, 2024
edited
Loading

Do I add them all?

Great find, and no, that sounds like overkill. Let's break out a function for windows env as I mentioned in my code comment, and then only provide the non-CSIDL prefixed vars.

Thank you for doing the due diligence here.

@JCHacking
Copy link
ContributorAuthor

  • Extracted to another method
  • Added the rest of environment variables
  • Add only on windows systems

It is not documented but I have added PATHEXT, this environment variable tells which are the extensions of the files that can be executed, I think it is useful.

If you are ok with the way the code is now, I think about how we could test this (probably parameterized with sys.platform).

@JCHacking
Copy link
ContributorAuthor

Hi@codejedi365
I have added the tests and documentation.

The testtest_version_runs_build_command now says it is linux and the system has windows environment variables to make sure that in case it is a non-windows system they are not passed.

And I have created another testtest_version_runs_build_command_windows with the scenario of a windows system.

@codejedi365
Copy link
Contributor

It is not documented but I have added PATHEXT, this environment variable tells which are the extensions of the files that can be executed, I think it is useful.

It seems to be documented here related to powershell:https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-7.4

Copy link
Contributor

@codejedi365codejedi365 left a comment

Choose a reason for hiding this comment

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

Great work, thank you for putting in the time and effort. Just a few aesthetics remain, sorry for the delay in reply.

@JCHacking
Copy link
ContributorAuthor

Formatted with ruff and changed tests to use patch_os_environment.
Are there any other changes needed? I think I read about using the windows check in get_windows_env, but now I can't find the message, do you want me to change it?

Copy link
Contributor

@codejedi365codejedi365 left a comment

Choose a reason for hiding this comment

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

Great thank you for the updates. We can leave what you have for theis_windows ternary statement.

@JCHacking
Copy link
ContributorAuthor

Great, then I see it as ready for me too.

Let me know when this is versioned so I can update the library in my developments.

Thank you very much!!!

codejedi365 reacted with thumbs up emoji

@codejedi365codejedi365 merged commitd911fae intopython-semantic-release:masterJun 5, 2024
@codejedi365codejedi365 added bugSomething isn't working properly released labelsJun 5, 2024
@codejedi365
Copy link
Contributor

codejedi365 commentedJun 5, 2024
edited
Loading

🎉 This PR was released in version 9.8.1 🎉

The release is available on:

JCHacking reacted with hooray emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@codejedi365codejedi365codejedi365 approved these changes

Assignees

No one assigned

Labels

bugSomething isn't working properlyreleased

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JCHacking@codejedi365@jmencianaranjo-ionos

[8]ページ先頭

©2009-2025 Movatter.jp