citrix-workspace: 24.5.0.76 -> 24.8.0.98#360106
Conversation
github-actions
Bot
added
the
8.has: maintainer-list (update)
flacks
force-pushed
the
citrix-workspace
branch
from
e324901 to
4719c67
Compare
NixOSInfra
added
the
12.first-time contribution
flacks
force-pushed
the
citrix-workspace
branch
from
4719c67 to
32756ad
Compare
flacks
changed the title
flacks
force-pushed
the
citrix-workspace
branch
from
32756ad to
917528a
Compare
ofborg
Bot
added
8.has: package (new)
SigmaSquadron
left a comment
SigmaSquadron
left a comment
There was a problem hiding this comment.
Welcome to Nixpkgs; thanks for adopting this package!
Looks good to me overall, but I didn't understand why have the || true expression in the installPhase.
There was a problem hiding this comment.
If I'm understanding this correctly, some of these files no longer exist. Can the wildcard instead be altered so these files aren't passed to rm?
There was a problem hiding this comment.
Since this package supports lots of older versions of Citrix (see sources.nix), I'm not sure if those are present in the older packages, hence the || true. I have not tested anything older than the last 2 releases as of this writing. I'm assuming some enterprises require older versions of Citrix to operate, given how slow businesses are to adopt newer versions sometimes. If those files are present for older Citrix versions, they may fail to launch. What should we do?
There was a problem hiding this comment.
My question spawned from my ignorance about how the generic Citrix builder works. This makes sense, and it's fine the way it is.
There was a problem hiding this comment.
It took me a good while to understand how this package works/builds. Hopefully it can be cleaned up more in the future :)
SigmaSquadron
added
the
0.kind: package adoption
There was a problem hiding this comment.
Please rename the maintainer commit to:
maintainers: add flacks
There was a problem hiding this comment.
Won't this fail with earlier versions that use 407?
There was a problem hiding this comment.
Sigh, you're right. Maybe we should add fail-safes for both 407 and 410? Using something like || true here too?
There was a problem hiding this comment.
Personally I'd delete this entire package and start anew with supporting just the latest version :)
I too wanted to maintain four different versions of an unmaintained package, but I was lucky enough to have others convince me that's a terrible idea.
If you don't want to do that yet, then || true works.
There was a problem hiding this comment.
I would like to get this in so I don't have to maintain out-of-tree patches in my local tree for building my local config. I went ahead and did a || true for both 407 and 410 for now, which builds fine locally. You're right that this package should be rewritten with just the latest version in mind... it is a terrible idea and a huge burden to do a take-your-pick for package versions. It doesn't even seem like Citrix has links to the older versions available for download (https://www.citrix.com/downloads/workspace-app/linux/) ... I will take up rewriting it some weekend in the near future :)
There was a problem hiding this comment.
Just looked at those Xen changes, my condolences. Can we maybe change every supported version for this package to have a different crying emoji appended to all of the binaries and desktop file?
flacks
force-pushed
the
citrix-workspace
branch
from
917528a to
1a6674a
Compare
flacks
force-pushed
the
citrix-workspace
branch
from
1a6674a to
d57718d
Compare
SigmaSquadron
left a comment
SigmaSquadron
left a comment
There was a problem hiding this comment.
LGTM!
Remember that as the maintainer, you can always decide to nuke this package and support only the versions you actually use.
SigmaSquadron
added
the
12.approvals: 1
|
Roger, thank you very much. Also, can I reformat everything using nixfmt-rfc-style? I saw that a tree-wide reformatting is coming soon, and I believe it's with the RFC style formatter? |
|
Feel free to do so in a separate commit, but as you stated, a treewide will take care of it for you. |
|
Ok. Will just format when I get around to the rewrite then. |
4 participants
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.