Skip to content

Remove unnecessary executable permissions from example and test scripts - #9133

Open
Esteban82 wants to merge 5 commits into
masterfrom
fix-9048
Open

Remove unnecessary executable permissions from example and test scripts#9133
Esteban82 wants to merge 5 commits into
masterfrom
fix-9048

Conversation

@Esteban82

@Esteban82 Esteban82 commented Aug 21, 2026

Copy link
Copy Markdown
Member

Assisted-by: Claude Sonnet 5 (High effort)

Removed the executable permission from example and test .sh scripts under doc/ and test/.
Updated the code-validator.yml CI checker to reflect this new policy.

Closes #9048

@Esteban82
Remove unnecessary executable permissions from example and test scripts
5185bfc 
All *.sh scripts under doc/ and test/ are invoked through an interpreter
(e.g., `bash gmtest ex01.sh`), so the executable bit is not needed and
causes issues on Windows checkouts (#9047). Scripts genuinely run
directly, such as test/geodesy/run_GPS_case_sub, keep their permissions.

Closes #9048
@Esteban82
ci: update code-validator to match new script permission policy
c3ad210 
The "Check execute permission of bash scripts" step required every
*.sh file to be executable, which now conflicts with the previous
commit removing the executable bit from doc/ and test/ scripts.
Adapt it to require execute permission everywhere else, and to
require the *absence* of it under doc/ and test/.

Related to #9048
@Esteban82
Esteban82 requested review from joa-quim and seisman August 21, 2026 13:48
@Esteban82 Esteban82 added the AI-assisted All (or most) of the code was written by Artificial Intelligence. label Aug 21, 2026
@seisman

seisman commented Aug 22, 2026

Copy link
Copy Markdown
Member

I don't think these scripts need executable permissions. They can be run via bash xxx.sh.

$ find . -name "*.sh" ! -path "./doc/*" ! -path "./test/*" \
            ! -path "./share/tools/gmt_functions.sh"
./ci/config-gmt-windows.sh
./ci/config-gmt-unix.sh
./ci/download-coastlines.sh
./ci/install-dependencies-linux.sh
./ci/install-dependencies-macos.sh
./ci/simple-gmt-tests.sh
./ci/deploy-gh-pages.sh
./ci/build-gmt.sh
./ci/install-dependencies-windows.sh
./admin/src_replace_word.sh
./admin/src_convention_check.sh
./admin/build-scientific-colors-cpt.sh
./admin/build-macos-external-list.sh
./admin/copyright_year.sh
./admin/list_known_failures.sh
./admin/remote_files_used.sh
./admin/run_cppcheck.sh
./admin/git_release_history.sh
./admin/image_optimize.sh
./admin/gs_check.sh
./admin/build-macos-cmake-cache.sh
./admin/gmt_make_pattern_include.sh
./admin/build-cmocean-cpt.sh
./admin/examine-failure-log.sh
./admin/build-release.sh
./admin/place-release.sh
./share/tools/gmt_links.sh
./share/tools/gmt_prepmex.sh
./share/tools/gmt_uninstall.sh
./share/tools/christmas-cube.sh
./src/gsfml/fz_funcs.sh
./src/gmt_make_PSL_strings.sh
./src/gmt_make_enum_dicts.sh
./src/gmt_make_module_purpose.sh
./src/spotter/spotter.sh
./src/shelltest.sh
./src/windbarbs/test/grdbarb_example.sh
./src/windbarbs/test/psbarb_example.sh
./src/mgd77/make_new_dst.sh
./src/mgd77/reformat_kyoto_dst.sh
./src/mgd77/mgd77netcdfhelper.sh
./src/mgd77/mgd77_codes.sh
./src/x2sys/test_x2sys.sh
./src/gmt_shell_functions.sh

@Esteban82
Extend executable-permission removal to all remaining bash scripts
dfd360e 
Per feedback from @seisman on PR #9133: every *.sh script in the repo
is run through 'bash', not directly, so none of them need execute
permission except share/tools/gmt_functions.sh, which is meant to be
sourced. This covers scripts under admin/, ci/, share/tools/, and src/
that were left untouched in the previous commit. Simplify the
code-validator.yml check accordingly, back to a single rule.

Assisted-by: Claude Sonnet 5 (High effort)
@seisman

seisman commented Aug 22, 2026

Copy link
Copy Markdown
Member

gmt_functions.sh is expected to be used like source gmt_functions.sh, so it also doesn't need executable permission.

@Esteban82
Remove executable permission from gmt_functions.sh too
9b76e66 
Per feedback from @seisman on PR #9133: gmt_functions.sh is meant to
be used as 'source gmt_functions.sh', so it doesn't need execute
permission either. Drop the last exception from the code-validator.yml
check.

Assisted-by: Claude Sonnet 5 (High effort)
@Esteban82

Esteban82 commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

Fixed now. No more exceptions, no *.sh should be executable.

@seisman

seisman commented Aug 22, 2026

Copy link
Copy Markdown
Member

The Tests workflow fails and needs to be fixed. it can be done in a separate PR because this PR already contains too many changed files.

@Esteban82

Esteban82 commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

The Tests workflow fails and needs to be fixed. it can be done in a separate PR because this PR already contains too many changed files.

They are failing because of this PR, right?

@seisman

seisman commented Aug 22, 2026

Copy link
Copy Markdown
Member

I think we need to update

gmt/test/gmtest.in

Line 309 in 6312737

. "${local_script}"

to

bash "${local_script}"

@Esteban82

Esteban82 commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

It sounds fine. I will do make another PR for this.

@Esteban82 Esteban82 mentioned this pull request Aug 22, 2026
Merged
Esteban82 added a commit that referenced this pull request Aug 22, 2026
@Esteban82
test: gmtest no longer requires execute permission on test scripts (#…
c78943d 
…9135)

Test scripts are sourced (". ${local_script}"), not executed, so the
existing "if ! [ -x "${script}" ]" guard was checking the wrong bit.
It started failing every test once #9133 removed the unnecessary
executable permission from *.sh scripts. Relax the guard to check
readability instead.

Note: switching the sourcing itself (". ${local_script}") to
"bash ${local_script}" was also proposed, but that would run the test
script in a subshell instead of gmtest.sh's own shell, breaking the
"ps=" variable classic-mode scripts set for later baseline comparison
(it wouldn't propagate back). Sourcing is kept as-is since it never
required execute permission in the first place.

Related to #9048, #9133

Assisted-by: Claude Sonnet 5 (High effort)
@joa-quim

joa-quim commented Aug 22, 2026

Copy link
Copy Markdown
Member

I don’t understand why is needed this to change all these files. If the solution is to call them prefixed with bash it doesn’t matter if they have the executable mode or not.

@seisman

seisman commented Aug 23, 2026

Copy link
Copy Markdown
Member

As I understand it, scripts created under Windows do not have executable permissions by default. Consequently, we may encounter inconsistent executable permissions across our scripts in the future.

@Esteban82

Esteban82 commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

I think Joaquim meant that it isn't necessary to remove the execution permission from all existing .sh files. It makes sense to be consistent, but it isn't required.

@seisman

seisman commented Aug 23, 2026

Copy link
Copy Markdown
Member

Yes, not required, but better to be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joa-quim joa-quim Awaiting requested review from joa-quim
@seisman seisman Awaiting requested review from seisman

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

AI-assisted All (or most) of the code was written by Artificial Intelligence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove unnecessary executable permissions from Bash example and test scripts

3 participants

@Esteban82 @seisman @joa-quim