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

Support multiline blocks and inline commets on profiles #16329

Closed

Conversation

perseoGI
Copy link
Contributor

Changelog: Feature: Support multiline blocks and inline commets on profiles
Docs: https://github.com/conan-io/docs/pull/XXXX
Closes: #16328

  • 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.

@RubenRBS RubenRBS added this to the 2.4.0 milestone May 23, 2024
@perseoGI perseoGI force-pushed the 16328-enhance-profile-parsing branch from a5a5f92 to f4b793d Compare May 23, 2024 15:22
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.

Case broken by this PR, profile with

[replace_requires]
zlib/1.3: zlib/1.3#rrev1

We can try to be smart and think about all the other potential corner cases that would be breaking, and keep increasing the regex complexity to cover those, but there will always be more that we cannot simply foresee. Take into account that profiles also support jinja syntax, that in turn can contain practically any python code, so regex based replacer there will always be risky.

@perseoGI
Copy link
Contributor Author

Case broken by this PR, profile with

[replace_requires]
zlib/1.3: zlib/1.3#rrev1

We can try to be smart and think about all the other potential corner cases that would be breaking, and keep increasing the regex complexity to cover those, but there will always be more that we cannot simply foresee. Take into account that profiles also support jinja syntax, that in turn can contain practically any python code, so regex based replacer there will always be risky.

Yes, you are totally right!
I've modified the code to ignore comment function of # if it is not at least separated by a blank character from the end of the line.
All tests are passing now.

We may see if there is any other possible case where we could be breaking something.
I can't think of any at the moment.

In terms of breaking changes in the jinja python injected code... I think there can't be any issue.
E.g. In this template...

[settings]
os = {{ {"Darwin": "Macos"}.get(platform.system(), platform.system()) }}

there can't be any # inside (except inside a string -> which are correctly handled by the regex)
And there is also no possibility to put a \ at the end because a jinja template must always be surrounded by
{{ ... }} or {% ... %}

Also, and fortunately, jinja supports natively multiline blocks without needing to add \ at the end of each line so that is great!

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.

{# # my useless comment #}

include(default)

Will break your regex replacer again.

I didn't have to provide a very complex profile, just one line to break your code.

See for reference #15544 (comment) what some users are doing with profiles.

I think this is not worth the risk.

@memsharded
Copy link
Member

Note, that if you want to have a beautiful formatted dict, you can probably define such a dict in jinja, then render it as text in the extra_variables field.

@memsharded
Copy link
Member

Closing as risk is too high for little value.

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.

[feature] Allow multiline blocks and inline comments in profiles
3 participants