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

dnf5 -x=dnf install dnf does not work as expected #1353

Open
j-mracek opened this issue Mar 28, 2024 · 15 comments · May be fixed by #1434 or #1442
Open

dnf5 -x=dnf install dnf does not work as expected #1353

j-mracek opened this issue Mar 28, 2024 · 15 comments · May be fixed by #1434 or #1442
Assignees
Milestone

Comments

@j-mracek
Copy link
Member

-x=dnf adds exclude = =dnf to configuration. Try --dump-main-config to see it.

-x dnf works as expected

Proposing the bug as a bug with high priority, because the option is important and other options might be affected as well.

@m-blaha
Copy link
Member

m-blaha commented Mar 28, 2024

I think we do not support syntax with = for short options. Only for the long ones.

@ppisar
Copy link
Contributor

ppisar commented Mar 28, 2024

We don't. dnf5(8) manual agrees it's an invalid invocation. I would rather fix the option parser to reject "-x=dnf" as an invalid option.

@jan-kolarik
Copy link
Member

Also this behavior should be documented in the dnf4 vs dnf5 changes doc.

@jrohel
Copy link
Contributor

jrohel commented Mar 28, 2024

DNF5 command line options format:

Prefix of long option --.
The prefix of a list of short options -. The last option in the list may have a value.

Examples with the same meaning:

  • --assumeyes --exclude=dnf (preferred long format, unambiguous format)
  • --assumeyes --exclude dnf (alternative long format, For the parser to correctly decode the string dnf - is it a value of the --exclude option or the next argument - it must know whether or not the --exclude option expects a value.)
  • -yxdnf
  • -yx dnf
  • -y -xdnf
  • -y -x dnf
  • --assumeyes -xdnf
  • --assumeyes -x dnf
  • -y --exclude=dnf
  • -y --exclude dnf

In DNF5, there is already a lot of freedom when entering options on the command line.
Introducing another possibility, a short option with a value separated by = (as requested -x=dnf), would be really confusing.

@m-blaha
Copy link
Member

m-blaha commented Apr 2, 2024

In DNF5, there is already a lot of freedom when entering options on the command line. Introducing another possibility, a short option with a value separated by = (as requested -x=dnf), would be really confusing.

I agree. I'd prefer to not implement -x=package format in dnf5 and just document the difference (as dnf4 supports it).
The format without = is also inline with POSIX Utility conventions (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html)

@j-mracek
Copy link
Member Author

j-mracek commented Apr 2, 2024

Be honest -yxdnf looks confusing. Anyway it might be a compatibility issue. It looks like that RPM supports short
options with -EARCH, -E ARCH and -E=ARCH

@j-mracek
Copy link
Member Author

j-mracek commented Apr 2, 2024

I see as a problem that DNF5 is not unified in using option separators.

--exlude=dnf works but -x=dnf doesn't.

And with excludes you will not get any error, but only a different behavior that might be difficult to discover.

@j-mracek
Copy link
Member Author

j-mracek commented Apr 2, 2024

We don't. dnf5(8) manual agrees it's an invalid invocation. I would rather fix the option parser to reject "-x=dnf" as an invalid option.

This would be another option how to handle the issue.

j-mracek added a commit to j-mracek/dnf5 that referenced this issue Apr 22, 2024
It unifies behavioe with long options and provide compatibility
with DNF4 and rpm.

Closes: rpm-software-management#1353
@j-mracek j-mracek linked a pull request Apr 22, 2024 that will close this issue
@j-mracek j-mracek self-assigned this Apr 22, 2024
@jrohel
Copy link
Contributor

jrohel commented Apr 23, 2024

As a console user, I like applications to behave consistently. And when I wrote the argument parser, I wanted it to be consistent with the rest of the system. The = is not a value delimiter in the short options list in the system.

Is the fact that rpm parses the short option list differently than the other standard utilities on the system enough reason for us to do it differently too? I don't want to be surprised when one day I want to set a value with = at the beginning and it omits it as a delimiter.

I understand that it may be a surprise to a beginner that = is not a delimiter value in a short list of options, but to a user who normally works in the Linux console and uses the standard utilities, it's a fact.

Examples:

$ cut -f=2 test.txt 
cut: invalid field value '=2'
$ grep -A=4 needle
grep: =4: invalid context length argument
$ head -n=1 test.txt 
head: invalid number of lines: '=1'

$ tar -cf=something.tar something
creates the file =something.tar. The = will be part of the name and not a delimiter.

@j-mracek
Copy link
Member Author

j-mracek commented Apr 24, 2024

The patch (#1434) unifies the behavior with DNF4, RPM and other project that uses Python3 argparse (including CI_DNF_STACK). So far it looks like that it doesn't break anything. Anyway I am going to provide an alternative solution that raises an exception, because the silently skipping this situation looks like the worst option.

j-mracek added a commit to j-mracek/dnf5 that referenced this issue Apr 24, 2024
It unifies behavioe with long options and provide compatibility
with DNF4 and rpm.

Closes: rpm-software-management#1353
j-mracek added a commit to j-mracek/dnf5 that referenced this issue Apr 24, 2024
It prevents silently skipping  the change in behavior between DNF4
and DNF5 Where `-x=dnf` excluded `dnf` but in DNF5 it excludes `=dnf`.
Without the exception it would be hard to detect that something might
work differently then expected.

Closes: rpm-software-management#1353
j-mracek added a commit to j-mracek/dnf5 that referenced this issue Apr 24, 2024
It prevents silently skipping  the change in behavior between DNF4
and DNF5 Where `-x=dnf` excluded `dnf` but in DNF5 it excludes `=dnf`.
Without the exception it would be hard to detect that something might
work differently then expected.

Closes: rpm-software-management#1353
@j-mracek j-mracek linked a pull request Apr 24, 2024 that will close this issue
@j-mracek
Copy link
Member Author

Here is an alternative solution mentioned by Petr Pisar above - Reject = with short options - #1442.

@jrohel
Copy link
Contributor

jrohel commented Apr 24, 2024

I tested DNF4. And I found that it doesn't support the = delimiter in the short options list.
The = is only taken as a delimiter if just one short option is listed. The DNF4 parser has another strange rule. So -vx=dnf causes the exclude =dnf to be set. Same behaviour as in DNF5.
In other words, DNF5 is incompatible with DNF4 only if the short options list contains just one short option.

# dnf -vx=dnf install dnf -> Excludes in dnf.conf: =dnf
# dnf -vxdnf install dnf -> Excludes in dnf.conf: dnf
# dnf -v -x=dnf install dnf -> Excludes in dnf.conf: dnf
# dnf -v -xdnf install dnf -> Excludes in dnf.conf: dnf

@j-mracek
Copy link
Member Author

@jrohel I understand your point and I am not pushing any solution. I have one very strong concern - the change of behavior is silent therefore very difficult to discover.

So far we have 4 options

  1. Keep the code like it is and only document the change
    I consider this solution a risky because it would be easy to overlook the change because nothing will fail

  2. Do nothing (no comment)

  3. Raise an exception when = is the first character in an argument and document the change
    This will make the change transparent, but it will create a noise.

  4. Provide a compatible solution with DNF4

DNF and DNF5 does not validate configuration - string =dnf is not a valid name of RPM

@jan-kolarik
Copy link
Member

jan-kolarik commented Apr 30, 2024

I was searching for some existing standard related to this topic and found the following link: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html. According to 2. a., conforming applications should support both -xdnf and -x dnf variants. If we want to comply, we should drop support for the = delimiter for short options. I didn't find an explicit standard for long options mentioning the use of the = delimiter, but it seems generally accepted with other standard commands and is also supported by UNIX getopt.

Given the existing feedback, I'm personally in favor of not supporting = as a delimiter for short options. However, we should document this change clearly and include it in the dnf4 vs. dnf5 changes documentation. I'm not sure about always throwing an error when the = delimiter is used with short options, as it might be a valid value for some future option, as Jarek mentioned. Instead, I'd prefer a more consistent approach: if a specific character isn't valid for a particular option and needs validation, then let's throw an error, and it should apply consistently wherever the validation fails.

EDIT: Oh, Marek had already posted the IEEE link. Although I read his post, I somehow missed this. :)

@jan-kolarik jan-kolarik added this to the Fedora 41 milestone May 13, 2024
@jan-kolarik
Copy link
Member

Since there has been no feedback for some time, let's finalize our implementation approach.

It seems we agree on not supporting the = delimiter for short options. If so, we should document this properly in the changes docs, etc.

The last thing to decide is whether to throw an exception when the = character is used after a short option (see my previous comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
5 participants