falkTX / Cadence

Collection of tools useful for audio production
GNU General Public License v2.0
368 stars 80 forks source link

Rename xinit script to match best practices #305

Closed ratijas closed 3 years ago

ratijas commented 3 years ago

Dash after number provides visual clarity, and .sh suffix helps text editors with file extension-based syntax highlighting engines.

ratijas commented 3 years ago

At first I committed via GitHub web flow (hence patch-1 branch name), but then I realized there might be danglig references to the file, so I cloned repo locally and ensured all mentions (namely, 3 occurrences in Makefile) are replaced as well. Should be good to go.

ratijas commented 3 years ago

Hi @falkTX,

Could you please take a look at this PR?

ratijas commented 3 years ago

Ping, @falkTX

falkTX commented 3 years ago

sure sorry for delay.

renaming the file is problematic for distributions that keep /etc files from previous installs, and any user that installs manually.

can you extend the check at the top of the file? like:

if [ -f $INSTALL_PREFIX/bin/cadence-session-start ] && [ ! -e /path/to/61cadence-session-inject ]; then

this way the new file does nothing if the old one is still there

ratijas commented 3 years ago

distributions that keep /etc files from previous installs, and any user that installs manually

God save those poor souls

this way the new file does nothing if the old one is still there

So that the new code won't be executed in favor of legacy? That's extremely bad for maintenance and any future modifications. Should've at least print a warning then.

falkTX commented 3 years ago

What changes does this really need? The actual code that runs is in python, this is just to inject into the startup, does not need to change.

ratijas commented 3 years ago

Idk, but I could imagine such an extra check giving someone one hell of a sleepless debugging night while trying to reproduce some weird github issue.

Otherwise, it's just as the title says: to be consistent with the adopted naming scheme on Linux platform.

ratijas commented 3 years ago

I double checked the script, and it looks idempotent. I.e. there should be no harm if it executes twice, except that old version might get executed last, thus potentially overwriting env with junk:

$ print "61-ca\n61cad" | sort
61-ca
61cad

This shouldn't be happening in the first place. And if/when it happens, there's not much we can do. I don't consider this to be a problem that deserves bikeshedding like hard-coding legacy filenames and other weird stuff.


OK, actually there is a misterious line which goes like STARTUP=".../cadence-session-start ...$STARTUP", and I'm yet to figure out the meaning of it. Maybe it's some kind of magic for some DE? Idk, neither my ~/.xinitrc nor KDE doesn't seem to use it in any way. This is what my ~/xinitrc looks like anyway:

# start some nice programs
if [ -d /etc/X11/xinit/xinitrc.d ] ; then
 for f in /etc/X11/xinit/xinitrc.d/?*.sh ; do
  [ -x "$f" ] && . "$f"
 done
 unset f
fi

It's been copied from default script and stripped down years ago, per recommendations at Arch Linux wiki1.

ratijas commented 3 years ago

@ratijas ratijas force-pushed the ratijas:patch-1 branch from adc9110 to bc65e72 now

Found and fixed one reference to *cadence-session-inject* script.

ratijas commented 3 years ago

Uhm, @falkTX

Turned out, there's actually a clean up script for what you were asking.

Cadence/Makefile:

  189   # Delete old files
  190:  rm -f $(X11_RC_DIR)/21cadence-session-inject
  191   rm -f $(X11_RC_DIR)/70cadence-plugin-paths
  192   rm -f $(X11_RC_DIR)/99cadence-session-start
  ...
  215   rm -f $(DESTDIR)$(PREFIX)/share/icons/hicolor/scalable/apps/claudia-launcher.svg
  216   rm -f $(DESTDIR)/etc/xdg/autostart/cadence-session-start.desktop
  217:  rm -f $(X11_RC_DIR)/61-cadence-session-inject.sh
  218   rm -rf $(DESTDIR)$(PREFIX)/share/cadence/
  219  
  220   # Old stuff
  221:  rm -f $(X11_RC_DIR)/21cadence-session-inject
  222   rm -f $(X11_RC_DIR)/70cadence-plugin-paths
  223   rm -f $(X11_RC_DIR)/99cadence-session-start

I was blind not to notice it right below the line that I've changed.

falkTX commented 3 years ago

ah right, because this has happened before already.

hmm since cadence is not really packaged in debian, I guess this is fine then.

falkTX commented 3 years ago

Can you add a line to delete the "old" 61cadence-session-inject? thanks!

ratijas commented 3 years ago

Done and done!

falkTX commented 3 years ago

Thanks again

ratijas commented 3 years ago

Feels good to have it sorted out :)