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

wireshark: 3.2.1 -> {2.6.12, 3.0.7, 3.2.1} #68926

Closed
wants to merge 1 commit into from
Closed

Conversation

@volth
Copy link
Contributor

volth commented Sep 16, 2019

Motivation of restoring 2.x is:

  1. libwiretap has breaking API changes in 3.0
  2. 2.4 and 2.6 branches are active upstream
    2.4.16 is two months old, 2.6.11 is 5 days old

cc @teto

@volth volth force-pushed the volth:wireshark2 branch from 34b5be7 to a746124 Sep 16, 2019
@ofborg ofborg bot requested review from bjornfor and fpletz Sep 16, 2019
@teto
Copy link
Contributor

teto commented Sep 16, 2019

Regarding the motivations, we don't seem to have packages that depend on wiretap ?
and even if old packages are maintained, it doesn't mean, we need to provide them, wireshark 3 should be superior in every way to wireshark 2.

The motivation seem to be #56776 (comment) . Can't panda upgrade to the latest wireshark ?
The increased complexity looks fine/manageable but better if we could do without it.

@volth
Copy link
Contributor Author

volth commented Sep 16, 2019

wireshark 3 should be superior in every way to wireshark 2

only in a way as a GUI program, not as an incompatible C library
kind of superiority of gtk 3 over gtk 2

@volth volth force-pushed the volth:wireshark2 branch from 8d3554f to 6b4e51f Sep 16, 2019
@volth
Copy link
Contributor Author

volth commented Sep 16, 2019

Although I do not mind if the GUI program (wireshark-qt) will be only 3.0 to save Hydra cycles.
All I need from wireshark 2 is dev files

@volth
Copy link
Contributor Author

volth commented Oct 1, 2019

rebased

@teto
Copy link
Contributor

teto commented Oct 2, 2019

is that ok to merge a merge ? as in nixos policiy, I am afraid it will break some workflows. I tend to prefer rebased commits.
Do you need the 2.4 too ? trying to save hydra cycles should be necouraged, if someone needs the 2.6 gui we can revisit.

@volth volth force-pushed the volth:wireshark2 branch from 9131b5a to de3f797 Oct 2, 2019
@volth volth changed the title wireshark: 3.0.3 -> {2.4.16, 2.6.11, 3.0.4} wireshark: 3.0.5 -> {2.4.16, 2.6.11, 3.0.5} Oct 16, 2019
@fpletz
Copy link
Member

fpletz commented Oct 26, 2019

Since the 2.4 branch is not listed on the Wireshark download page and with 3.0 listed as stable and 2.6 as old stable, I would prefer if we don't add 2.4 at all.

Apart from that the PR looks good, thanks! We probably should also backport this to ease backporting package bumps in the future.

@volth
Copy link
Contributor Author

volth commented Oct 26, 2019

Yes, it must be {2.6.12, 3.0.6, 3.1.0} now

It would be nice if @r-ryantm updates PRs too :)

@volth volth force-pushed the volth:wireshark2 branch from de3f797 to ce4e558 Oct 26, 2019
@volth volth changed the title wireshark: 3.0.5 -> {2.4.16, 2.6.11, 3.0.5} wireshark: 3.0.5 -> {2.6.12, 3.0.6, 3.1.0} Oct 26, 2019
@teto
Copy link
Contributor

teto commented Nov 25, 2019

sorry i intended to merge this but the merge of master into it is an issue. Please use a rebase instead ? and I'll merge

@volth volth force-pushed the volth:wireshark2 branch from 7d439b1 to f6a80af Nov 25, 2019
@volth volth changed the title wireshark: 3.0.5 -> {2.6.12, 3.0.6, 3.1.0} wireshark: 3.0.5 -> {2.6.12, 3.0.6, 3.1.1} Nov 25, 2019
Copy link
Contributor

jtojnar left a comment

I do not really like including multiple versions of software unless it is required by something in nixpkgs. Users can always override locally when they need a different version.

If someone really needs it, we should limit it to the exact version required by the project, open upstream issue about upgrading, and make it clear in the source code when the need for it expires (e.g. by linking the issue). I do not see why would we need multiple GUI versions.

Also the duplication of hashes between the Qt and CLI variants is ugly.

@conferno
Copy link
Contributor

conferno commented Nov 25, 2019

the duplication of hashes between the Qt and CLI variants is ugly.

The very existing of Qt and CLI variants (and thus identical wireshark-cli.dev and wireshark-qt.dev) built from the same sources are ugly, there should be a single multiple-output derivation with
wirestark3.cli, wireshark3.qt, wireshark3.dev (and another with wirestark2.cli, wireshark2.qt, wireshark2.dev)

@volth volth changed the title wireshark: 3.0.5 -> {2.6.12, 3.0.6, 3.1.1} wireshark: 3.0.5 -> {2.6.12, 3.0.7, 3.2.0} Jan 15, 2020
@volth volth force-pushed the volth:wireshark2 branch from 789342d to 15a283a Jan 16, 2020
@volth volth force-pushed the volth:wireshark2 branch from 15a283a to 421330f Jan 16, 2020
@volth volth changed the title wireshark: 3.0.5 -> {2.6.12, 3.0.7, 3.2.0} wireshark: 3.2.1 -> {2.6.12, 3.0.7, 3.2.1} Jan 16, 2020
@Infinisil
Copy link
Member

Infinisil commented Feb 5, 2020

@volth Since it doesn't seem to be needed in nixpkgs, how about maintaining the 2.x version you need outside of nixpkgs? NUR could be used if other people are interested in such an older version too.

@volth
Copy link
Contributor Author

volth commented Feb 5, 2020

Well, how about at least fixing optionalString withQt logic in postInstall ?
currenly wireshark.dev has no include files

Or wireshark.dev doesn't seem to be needed in nixpkgs too?

@Infinisil
Copy link
Member

Infinisil commented Feb 5, 2020

Oh the fixes are great. I'm just talking about the part of this PR that adds the older versions, which apparently some people don't like. Though after reconsideration, I think an older version should be fine in this case, given that at least @volth is in need of it, which probably means other people are too, in addition to the fact that it's maintained upstream. And a package being used in nixpkgs was never a requirement for including packages in nixpkgs anyways.

@fpletz @jtojnar Would you reconsider? I think it would be fine to add at least one older version, given the circumstances

Though I don't get why 3.0.7 would be needed, can you explain @volth? Having multiple versions is a burden, so we shouldn't introduce them without good reasons.

@stale
Copy link

stale bot commented Aug 3, 2020

Hello, I'm a bot and I thank you in the name of the community for your contributions.

Nixpkgs is a busy repository, and unfortunately sometimes PRs get left behind for too long. Nevertheless, we'd like to help committers reach the PRs that are still important. This PR has had no activity for 180 days, and so I marked it as stale, but you can rest assured it will never be closed by a non-human.

If this is still important to you and you'd like to remove the stale label, we ask that you leave a comment. Your comment can be as simple as "still important to me". But there's a bit more you can do:

If you received an approval by an unprivileged maintainer and you are just waiting for a merge, you can @ mention someone with merge permissions and ask them to help. You might be able to find someone relevant by using Git blame on the relevant files, or via GitHub's web interface. You can see if someone's a member of the nixpkgs-committers team, by hovering with the mouse over their username on the web interface, or by searching them directly on the list.

If your PR wasn't reviewed at all, it might help to find someone who's perhaps a user of the package or module you are changing, or alternatively, ask once more for a review by the maintainer of the package/module this is about. If you don't know any, you can use Git blame on the relevant files, or GitHub's web interface to find someone who touched the relevant files in the past.

If your PR has had reviews and nevertheless got stale, make sure you've responded to all of the reviewer's requests / questions. Usually when PR authors show responsibility and dedication, reviewers (privileged or not) show dedication as well. If you've pushed a change, it's possible the reviewer wasn't notified about your push via email, so you can always officially request them for a review, or just @ mention them and say you've addressed their comments.

Lastly, you can always ask for help at our Discourse Forum, or more specifically, at this thread or at #nixos' IRC channel.

@stale stale bot added the 2.status: stale label Aug 3, 2020
@volth volth closed this Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.