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

[bugfix] Environment shell script path is now formatted based on the subsystem #11981

Conversation

jellespijker
Copy link
Contributor

The shell script for the environments should be formatted based on the subsystem now.

Fixes #11980

Changelog: (Bugfix): Environment shell script path is now formatted based on the subsystem
Docs: https://github.com/conan-io/docs/pull/XXXX

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

The shell script for the environments should be formated based on
the subsystem now.

Fixes conan-io#11980
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This would need a test that would be failing without this fix. I am concerned that this apparently innocent fix could be breaking other users, unfortunately the subsystems are not sufficiently tested.

We need to have a look to the whole subsystems thing, as it is still not a good fit. We have some ongoing experimentation efforts in #11359 and #11066, but nothing that looks good enough so far...

@jellespijker
Copy link
Contributor Author

Yes I agree with the risk there, I will investigate this in more detail. Also try out how it behaves in more user cases.

The new win-bash approach looks promising btw

@jellespijker
Copy link
Contributor Author

@memsharded the failing test occurred in #11987 which turned out to be a non issue. I do think this PR is save to merge and actually fixes the bug in described in #11980

@jellespijker
Copy link
Contributor Author

jellespijker commented Sep 14, 2022

@memsharded / @lasote has this fix been considered to make it to the next release. The error you were concerned about happened in another PR (which is now closed since it turned out to be a non-issue) This fix is relatively safe and contained imo.

And I think I will run into issues with some open PR's:

@jellespijker
Copy link
Contributor Author

As my colleague @Joeydelarago pointed out, the hint is in the comment:

This would need a test that would be failing without this fix.

I will write a testcase for this PR

@memsharded
Copy link
Member

Hi @jellespijker

It has been some time, maybe this was no longer a priority? Failed to write the testcase?
It is true that it is weird that subsystem is not passed there, but on the other hand, there have been no reports about this, so it seems subsystems are fine with the native path? It would be good to know a bit better, have failing tests for this.

@jellespijker
Copy link
Contributor Author

I'm fine with closing this PR. Our initial work, which required this fix, didn't end up in our production env. So this isn't PR isn't need by us anymore.

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] paths for environment files are not formatted for subsystem when run in win_bash on Windows
3 participants