Skip to content

Conversation

yarikoptic
Copy link
Collaborator

@yarikoptic yarikoptic commented Sep 22, 2025

Without the fix, on the system with POSIX compliant /bin/sh (dash etc) you could get

❯ make install
/bin/sh: 1: [[: not found
MKDIR	/home/yoh/.local/share/gnome-shell/extensions
LINK	paperwm@paperwm.github.com

INSTALL SUCCESSFUL:

If this is the first time installing PaperWM, then please logout/login
and enable the PaperWM extension, either with the GNOME Extensions application,
or manually by executing the following command from a terminal:

gnome-extensions enable paperwm@paperwm.github.com

after a fix

❯ make install

INSTALL FAILED:

A previous (non-symlinked) installation of PaperWM already exists at:
'/home/yoh/.local/share/gnome-shell/extensions/paperwm@paperwm.github.com'.

Please remove the installed version from that path and re-run this install script.

make: *** [Makefile:40: install] Error 1

and I think I will follow up with an additional fix now for using -e so those fail overall whenever first error happens instead of keeping plowing through! ...

actually such patch didn't work as it kept plowing through after the error
diff --git a/Makefile b/Makefile
index e51dde7..441e78e 100644
--- a/Makefile
+++ b/Makefile
@@ -30,14 +30,14 @@ else
 GNOME_EXT_DISABLE := gnome-shell-extension-tool --disable
 endif
 
-SHELL=/bin/bash
+SHELL=/bin/bash -e
 
 ## Update compiled files
 all: $(RELEASE_FILES)
 
 ## Install PaperWM on this system
 install: schemas/gschemas.compiled
-	@if [[ ! -L "$(TARGET)" && -d "$(TARGET)" ]]; \
+	@if [[[ ! -L "$(TARGET)" && -d "$(TARGET)" ]]; \
 	then                                    \
 		echo;                               \
 		echo "INSTALL FAILED:";             \
❯ make install
/bin/bash: line 1: [[[: command not found
MKDIR	/home/yoh/.local/share/gnome-shell/extensions
LINK	paperwm@paperwm.github.com

INSTALL SUCCESSFUL:

If this is the first time installing PaperWM, then please logout/login
and enable the PaperWM extension, either with the GNOME Extensions application,
or manually by executing the following command from a terminal:

gnome-extensions enable paperwm@paperwm.github.com

changes on filesystem:                                                                                                                                                                                                            
 Makefile | 4 ++--

so might be better to just make them proper outside bash scripts and invoke here ... will not do in this PR

Without the fix, on the system with POSIX compliant /bin/sh (dash etc)
you could get

    ❯ make install
    /bin/sh: 1: [[: not found
    MKDIR	/home/yoh/.local/share/gnome-shell/extensions
    LINK	paperwm@paperwm.github.com

    INSTALL SUCCESSFUL:

    If this is the first time installing PaperWM, then please logout/login
    and enable the PaperWM extension, either with the GNOME Extensions application,
    or manually by executing the following command from a terminal:

    gnome-extensions enable paperwm@paperwm.github.com
Thesola10
Thesola10 previously approved these changes Sep 22, 2025
@smichel17
Copy link
Member

so might be better to just make them proper outside bash scripts and invoke here ... will not do in this PR

Would it make sense to revert #1011 instead (gets us back separate shell scripts) and then apply a different fix for cleaning up the root repo (e.g. move the scripts into a subfolder, add a wrapper makefile)?

smichel17
smichel17 previously approved these changes Sep 27, 2025
Copy link
Member

@smichel17 smichel17 left a comment

Choose a reason for hiding this comment

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

In the meantime, this is harmless.

@smichel17
Copy link
Member

@yarikoptic I've added you as a maintainer, so you may now merge your own PRs (once they are approved). You can also approve other people's PRs so they can merge, if you want to pay it forward :)

@yarikoptic
Copy link
Collaborator Author

so might be better to just make them proper outside bash scripts and invoke here ... will not do in this PR

Would it make sense to revert #1011 instead (gets us back separate shell scripts) and then apply a different fix for cleaning up the root repo (e.g. move the scripts into a subfolder, add a wrapper makefile)?

oh... and yeah, I would have kept helper scripts under tools/ or utils/ ... if ci specific, under ci/ subfolder there (at least this is how we do in some projects)

@yarikoptic I've added you as a maintainer, so you may now merge your own PRs (once they are approved). You can also approve other people's PRs so they can merge, if you want to pay it forward :)

oh. Not sure I would have enough time to contribute notably, but I do appreciate the trust!

But I guess I also need to RTFM since I see now

image

should I rebase for master or ... ?

@smichelForHire smichelForHire mentioned this pull request Sep 28, 2025
@smichel17
Copy link
Member

I think it's because it's targeting the release branch. You should edit the PR and target develop instead.

image

@smichel17
Copy link
Member

I do appreciate the trust!

You're welcome :)

As a community project, because we need maintainers, the policy is "give out commit access easily, but protect important branches (develop, release, and gnome-*) so a new or malicious contributor can't unilaterally mess up important things. But if 2-3* people with access all agree that a PR should be merged, then you shouldn't need to wait on the "main" maintainer(s).

*Annoyingly, GitHub doesn't have a mechanism to apply different merge controls to PRs coming from people with access vs unrelated public accounts.

@smichel17 smichel17 changed the base branch from release to develop October 20, 2025 12:25
@smichel17 smichel17 dismissed stale reviews from Thesola10 and themself October 20, 2025 12:25

The base branch was changed.

@smichel17 smichel17 merged commit bccb303 into paperwm:develop Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants