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

breakages of rpm 4.13 boolean dependencies #174

Closed
XRevan86 opened this issue Mar 15, 2017 · 11 comments
Closed

breakages of rpm 4.13 boolean dependencies #174

XRevan86 opened this issue Mar 15, 2017 · 11 comments
Assignees

Comments

@XRevan86
Copy link

rpm 4.13 (which found its way to Tumbleweed recently) has introduced a new feature: boolean dependencies.
But spec-cleaner (and format_spec_file for that matter) messes them up, turning

Requires: (foo and bar)

into

BuildRequires:  (foo
BuildRequires:  and
BuildRequires:  bar)

The minimal (non-intrusive) mode does the same thing.

@sleep-walker
Copy link
Contributor

Code modification to accept boolean expressions is not hard and has not high priority to me. Patches are welcome.

dependency parser needs to be extended to:

  • understand beginning and end of (, ) braces - take whole content as single dependency
  • expect and and or operators and just stack them on found array without changing state

This should be sufficient.

@noor-fernandez
Copy link

I'm wondering why you didn't simply import the library regex instead of re? I believe that re is deprecated as of Python 3.6. Also, if you have an end of macro function why don't you have a beginning of macro function? Is it implicitly defined? Lastly, how are these boolean expressions being read by the parser. I realize that the parser is only reading one dependency at a time but do the two dependencies (foo, bar) look like this: Requires: (foo and bar) OR do they look like Requires: (%{foo} and %{bar)). That changes how the relevant functions work.
P.S. I did try to find the answer to my last question on google but no luck.

@sleep-walker
Copy link
Contributor

I'm wondering why you didn't simply import the library regex instead of re? I believe that re is deprecated as of Python 3.6.

I don't know that. Note that spec-cleaner is project older than Python 3.6. If you think it will improve the function, just make a patch.

Also, if you have an end of macro function why don't you have a beginning of macro function? Is it implicitly defined?

What would such function do? Look for %? Where would that be used? Patch?

Lastly, how are these boolean expressions being read by the parser. I realize that the parser is only reading one dependency at a time but do the two dependencies (foo, bar) look like this: Requires: (foo and bar) OR do they look like Requires: (%{foo} and %{bar)). That changes how the relevant functions work.

I'm probably a bit lost here. The function is not implemented and my last comment is naive description how it could be implemented. There are two missing things

  • support for looking for matching ) for ( which is not in the middle of some symbol (function is written but not used for this case) - we won't parse inner contents as there is no reason for that
  • support for and and or operators - the trick is to read more than one dependency at a time and before and after boolean operator do not flush what was already read.

What do you think about that?

@noor-fernandez
Copy link

noor-fernandez commented Apr 5, 2017

Your description is not naive and you are absolutely correct in saying that there are two things missing. When you say "symbol" do you mean a macro?
And yes we do need to read read two or more dependencies concurrently. I'm not sure I understand what "(function is written but not used for this case)" means.
Also, while updating re to regex is easy I can't manage to get the bug to come up in my code. That is, I don't see this: BuildRequires: (foo
BuildRequires: and
BuildRequires: bar)
I don't know how to change "macro = string[0:2]" which only eats the begging %{ as you said in your comment. so that your first point is satisfied.
let me know if i'm still confusing you.

@sleep-walker
Copy link
Contributor

Wow, I accidentally edited your comment instead of making my own - sorry.

Your description is not naive and you are absolutely correct in saying that there are two things missing. When you say "symbol" do you mean a macro?

I mean dependency - it can be package name or something package provides. For example pkgconfig(alsa) is symbol which has ( not in the beginning of the word and shouldn't be considered as a boolean expression.

And yes we do need to read read two or more dependencies concurrently. I'm not sure I understand what "(function is written but not used for this case)" means.

There is function find_end_of_macro which does exactly that, but is not used for boolean expressions. Code needs to be slightly adjusted as it expects % in the beginning.

Also, while updating re to regex is easy I can't manage to get the bug to come up in my code. That is, I don't see this: BuildRequires: (foo
BuildRequires: and
BuildRequires: bar)

If you are interested in implementing this, just add it as a test in tests/in/requires.spec and correct results to tests/out/requires.spec and tests/out-minimal/requires.spec.

I don't know how to change "macro = string[0:2]" which only eats the begging %{ as you said in your comment. so that your first point is satisfied.

IMHO choices are:

  • strip % before calling the function and change it to macro = string[0:1] and string = string[1:]
  • add next parameter to function find_end_of_macro like strip_leading_percent (can have default value True or False or no - depending on your taste)
    • split "consuming" % from consuming of { (or other opening brace)
    • make it conditional depending on strip_leading_percent.

let me know if i'm still confusing you.

It's much better after some sleep :)

@noor-fernandez
Copy link

Okay. I will actually do a patch for multiple python files for openSUSE repositories.
Thank you for my marching orders. :) Also, can I reword your """ opening statement """ to have a bit more flow? If not, I'll keep it as-is.
thanks for clarifying.

@sleep-walker
Copy link
Contributor

I'm not sure what you mean with that but it will be apparent from merge request, right? :)

@noor-fernandez
Copy link

So... in the first few lines of dependency parser.py you have a multi-line comment that says this:
"""RPM dependency lines parser and helpers.
Contains class DependencyParser which parses string and generates
token tree. For common manipulation is method flat_out() useful, it
just splits dependencies into list.
For future development is useful find_end_of_macro().
"""
I wanted to re-word it to say "the method flat_out() is useful for common manipulations. It splits dependencies into a list. The method find_end_of_macro() should be considered for future development."

@noor-fernandez
Copy link

But yeah, I'll get to work on it right away. :)

@noor-fernandez
Copy link

Okay, we might have a logic problem (perhaps not though) this is the modification I've made to depedency_parser.py:
def find_end_of_macro(string, regex, opening, closing, strip_leading_percent): #line 42
strip_leading_percent == '%' #lines 48-52
if strip_leading_percent == True:
macro = string[0:1]
# only eat opening '%'
string = string[1:]
now that macro and string have been redefined string is equal to opened because they both occupy the
position of 1 on the list. Is that a problem?

scarabeusiv pushed a commit that referenced this issue Jun 1, 2017
sleep-walker pushed a commit that referenced this issue Jun 7, 2017
@scarabeusiv
Copy link
Contributor

Code fixing this was merged in master.

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

No branches or pull requests

4 participants