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

make-desktopitem: refactoring, documentation and improvement #91790

Merged
merged 2 commits into from Oct 15, 2020

Conversation

piegamesde
Copy link
Member

@piegamesde piegamesde commented Jun 29, 2020

Motivation for this change
  • New parameter extraDesktopEntries to easily add some less usual entries to the desktop file
  • Rewrite of the core logic. Instead of a key-value-list, use an attribute set with nullable values to make it overridable
  • Added some comments
  • Some cosmetic/readability code refactors
    • I didn't like the doubly nested strings around the fileValidation
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

- New parameter `extraDesktopEntries` to easily add some less usual entries to the desktop file
- Rewrite of the core logic. Instead of a key-value-list, use an attribute set with nullable values to make it overridable
- Added some comments
- Some cosmetic/readability code refactors
	- I didn't like the doubly nested strings around the `fileValidation`
@mweinelt
Copy link
Member

Result of nixpkgs-review pr 91790 1

5 packages marked as broken and skipped:
- eagle7
- invoice2data
- pdfdiff
- pgadmin
- xpdf
24 packages failed to build:
- citrix_workspace (citrix_workspace_unwrapped ,citrix_workspace_unwrapped_20_04_0)
- citrix_workspace_19_10_0 (citrix_workspace_unwrapped_19_10_0)
- citrix_workspace_19_12_0 (citrix_workspace_unwrapped_19_12_0)
- citrix_workspace_19_6_0 (citrix_workspace_unwrapped_19_6_0)
- citrix_workspace_19_8_0 (citrix_workspace_unwrapped_19_8_0)
- frogatto
- ipmiview
- ostinato
- palemoon
- python37Packages.spyder
- spyder (python38Packages.spyder)
- quartus-prime-lite
- react-native-debugger
- sage
- sageWithDoc
- softmaker-office
- sqldeveloper
- stm32cubemx
- tdm
- termius
- toggldesktop
- tome2
- ut2004demo
- worldofgoo
168 packages built:
- _90secondportraits
- airtame
- alloy (alloy5)
- alloy4
- android-studio (androidStudioPackages.stable)
- androidStudioPackages.beta
- androidStudioPackages.canary
- androidStudioPackages.dev
- anydesk
- apache-directory-studio
- assaultcube
- avocode
- basex
- beneath-a-steel-sky
- betaflight-configurator
- bitwarden
- brogue
- broken-sword-25
- ccemux
- charles (charles4)
- charles3
- clipgrab
- cura_stable
- dbeaver
- ddccontrol
- discord
- discord-canary
- discord-ptb
- dolphinEmuMaster
- dosbox
- drascula-the-vampire-strikes-back
- dreamweb
- dropbox
- dropbox-cli
- eagle
- eclipses.eclipse-committers
- eclipses.eclipse-cpp
- eclipses.eclipse-java
- eclipses.eclipse-modeling
- eclipses.eclipse-platform
- eclipses.eclipse-scala-sdk
- eclipses.eclipse-sdk
- eduke32
- evilpixie
- flight-of-the-amazon-queen
- freeoffice
- frostwire
- ganttproject-bin
- ghidra-bin
- gitkraken
- gitter
- goattracker
- goattracker-stereo
- golden-cheetah
- gomuks
- gpsprune
- groove
- hakuneko
- hyperrogue
- ivan
- jabref
- jameica
- jd-gui
- jdiskreport
- jetbrains.clion
- jetbrains.datagrip
- jetbrains.goland
- jetbrains.idea-community
- jetbrains.idea-ultimate
- jetbrains.mps
- jetbrains.phpstorm
- jetbrains.pycharm-community
- jetbrains.pycharm-professional
- jetbrains.rider
- jetbrains.ruby-mine
- jetbrains.webstorm
- jitsi
- jmol
- joplin-desktop
- keen4
- keepass
- kodestudio
- leo-editor
- lighttable
- lure-of-the-temptress
- mame
- mari0
- mate.caja-dropbox
- mgba
- michabo
- mindustry
- minecraft
- mist
- mlterm
- monero-gui
- mrrescue
- munt
- netbeans
- netlogo
- nixui
- nottetris2
- obinskit
- open-ecard
- openjk
- orthorobot
- pdfsam-basic
- pharo
- pharo-launcher
- portfolio
- postman
- prusa-slicer
- pymol
- qpaeq
- rambox-pro
- ricochet
- rimshot
- riot-desktop
- robo3t
- rocksndiamonds
- rstudio
- rstudioWrapper
- runelite
- rxvt-unicode
- rxvt-unicode-unwrapped
- saleae-logic
- scid-vs-pc
- sidequest
- sienna
- simplenote
- slic3r
- slimerjs
- smartgithg
- squirrel-sql
- ssb-patchwork
- sublime
- svxlink
- sweethome3d.application
- sweethome3d.furniture-editor
- sweethome3d.textures-editor
- swingsane
- system-syzygy
- teamspeak_client
- teleprompter
- tensor
- thunderbird
- tlaplusToolbox
- todoist-electron
- tome4
- transgui
- trilium-desktop
- tusk
- tvbrowser-bin
- vapor
- vice
- vis
- visualvm
- vscode
- vscode-with-extensions
- vscodium
- wasabiwallet
- webbrowser
- winpdb
- wire-desktop
- write_stylus
- xmind
- xournal
- zotero
- zsnes

@piegamesde
Copy link
Member Author

As far as I can tell, this change does not break anything. The packages that don't build are either broken with compile errors, or broke because of #75729.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/247

@worldofpeace
Copy link
Contributor

Oops, I'll see if I can look at this.

Copy link
Member

@maralorn maralorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes a lot, let's merge them!

I have just a few nitpicks.

Besides that, let's see if there is any true dependency breakage. I feel confident enough to understand these changes to merge this, after we have reviewed the breakage.

pkgs/build-support/make-desktopitem/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/make-desktopitem/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/make-desktopitem/default.nix Outdated Show resolved Hide resolved
@maralorn
Copy link
Member

maralorn commented Oct 14, 2020

I think this is ready to merge as soon as someone can convince me, that this does not introduce any new breakage. (That'll probably be me, as soon a find the time to go through all build errors.)

@maralorn
Copy link
Member

Wait, now that I think about it: With the true instead of "true" change we might actually be introducing type errors. We need to find them and fix them in affected packages.

@piegamesde
Copy link
Member Author

Wait, now that I think about it: With the true instead of "true" change we might actually be introducing type errors. We need to find them and fix them in affected packages.

But why should it? We only only change the type of the default value, but the value itself still can be of any type.

But yes, maybe some beefy machine should run nixpkgs-review pr 91790 again to see if no packages broke in the meantime.

@maralorn
Copy link
Member

Wait, now that I think about it: With the true instead of "true" change we might actually be introducing type errors. We need to find them and fix them in affected packages.

But why should it? We only only change the type of the default value, but the value itself still can be of any type.

It should actually break on all packages, that have specified that option before, because they have all supplied a string, but we now expect a boolean. I didn‘t actually read your code. Apparently you have thought of that. Thanks.

(Fun fact: I have learned since yesterday, that is not specified with what terminal to open a desktop file and finding a standard for that is an at least 10 year old issue in gnome with a lot of bikeshedding on xdg lists…)

@piegamesde
Copy link
Member Author

It looks like that this pull request breaks palemoon, I'll look into it.

@maralorn
Copy link
Member

thx!

@maralorn
Copy link
Member

(For observers, we had figured out, that this does not break palemoon or any other of the above builds.)

piegamesde added a commit to piegamesde/nixpkgs that referenced this pull request Oct 15, 2020
This fixes all packages that are failed `nixpkgs-review` in NixOS#91790.
Packages that were broken prior to that PR were marked as broken.
Packages that failed because of NixOS#75729 were fixed.
cwyc pushed a commit to cwyc/home-manager that referenced this pull request Oct 24, 2020
This package depends on the makeDesktopItem function in nixpkgs, which recently changed its syntax:
NixOS/nixpkgs#91790

This commit makes the module compatible with the new syntax.

It also exposes the fileValidation option in makeDesktopItem.
cwyc pushed a commit to cwyc/home-manager that referenced this pull request May 22, 2021
This package depends on the makeDesktopItem function in nixpkgs, which recently changed its syntax:
NixOS/nixpkgs#91790

This commit makes the module compatible with the new syntax.

It also exposes the fileValidation option in makeDesktopItem.
cwyc pushed a commit to cwyc/home-manager that referenced this pull request Jun 2, 2021
This package depends on the makeDesktopItem function in nixpkgs, which recently changed its syntax:
NixOS/nixpkgs#91790

This commit makes the module compatible with the new syntax.

It also exposes the fileValidation option in makeDesktopItem.
berbiche pushed a commit to nix-community/home-manager that referenced this pull request Jun 2, 2021
* xdg-desktop-entries: add module

rebase

* xdg-desktop-entries: adapt to changes in makeDesktopItem

This package depends on the makeDesktopItem function in nixpkgs, which recently changed its syntax:
NixOS/nixpkgs#91790

This commit makes the module compatible with the new syntax.

It also exposes the fileValidation option in makeDesktopItem.

Co-authored-by: cwyc <cwyc@users.noreply.github.com>
Co-authored-by: --get <--show>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants