Brief: Add empty rules for pre-clean/post-clean #229

rwtolbert · 2025-08-14T10:53:43Z

Summary: The previous fix to add all these new rules did not check for the case where no pre-clean/post-clean was defined in a project.

as such, running janet-pm clean without defining pre-clean in the project.janet resulted in an error. The original tests all passed since all pre/post steps were defined.

This fix adds default empty rules
for each and then tests this in the test-bundle project that doesn't define pre-clean/post-clean.

Summary: The previous fix to add all these new rules did not check for the case where no pre-clean/post-clean was defined in a project. as such, running `janet-pm clean` without defining `pre-clean` in the project.janet resulted in an error. The original tests all passed since all pre/post steps were defined. This fix adds default empty rules for each and then tests this in the test-bundle project that doesn't define pre-clean/post-clean.

rwtolbert · 2025-08-14T11:16:39Z

I'm looking into why the build now fails. One problem I see is that even though a custom spork is installed in the random tmp directory, when the exec-slurp-all call uses os/spawn it doesn't know the special sys path and is installing test-project into the default Janet.

So I need to figure out how to spawn janet-pm with a custom sys path

rwtolbert · 2025-08-14T16:48:15Z

I'm closing in on another patch. When calling Janet-pm in my tests, it is ignoring the hard-coded syspath in the top of the file and then trying to install into the system, which of course it can't without permission.

I'm trying a fix that puts the hard-coded sys path inside the main where they do appear to affect the install location.

…ot used when run as main. This change adds a copy of the hardcoded paths inside a main in a binscript, if main exists

…_empty_rules

rwtolbert · 2025-08-15T18:07:48Z

This latest change adds code to fix an existing janet-pm problem.

and for some reason, the GitHub actions kicked off at an older SHA. It checked out 6f189ab0, which is the commit of the previous branch, before this PR, instead of f7d16f3 which is the latest change in this PR/branch.

https://github.com/janet-lang/spork/actions/runs/16996173637/job/48187304665

Run actions/checkout@v4
Syncing repository: janet-lang/spork
Getting Git version info
Temporarily overriding HOME='/home/runner/work/_temp/2511d18c-1bc3-475c-8796-59cd20e12e38' before making global git config changes
Adding repository directory to the temporary git global config as a safe directory
/usr/bin/git config --global --add safe.directory /home/runner/work/spork/spork/spork
Initializing the repository
Disabling automatic garbage collection
Setting up auth
Fetching the repository
Determining the checkout info
/usr/bin/git sparse-checkout disable
/usr/bin/git config --local --unset-all extensions.worktreeConfig
Checking out the ref
/usr/bin/git log -1 --format=%H
6f189ab0458075ac23f0d3306c8e9c870829ffe2

rwtolbert · 2025-08-15T18:13:23Z

@sogaiu @pyrmont Any ideas how to convince this to run the latest code? Is there something fundamentally wrong with my branch or this PR?

pyrmont · 2025-08-15T20:49:40Z

@rwtolbert You’re not going crazy. The issue seems to be due to the GitHub workflow using pull_request_target:

spork/.github/workflows/test.yml

Lines 3 to 7 in 6f189ab

    
           on: 
        
             push: 
        
               branches: [ master ] 
        
             pull_request_target: 
        
               branches: [ master ]

Based on this SO answer, this causes the test to run by checking out the commit of the target (i.e. the current HEAD of the master branch) rather than a merge commit that would become the current HEAD of the master branch.

The change was added in 05eb014, apparently as a way to get the workflow to run without needing a maintainer to approve it (see #178). If I’m understanding things correctly, this means that pull request checks are currently meaningless because all it’s doing is running the current HEAD of master.

As for the issue that #178 identified, I don’t have commit privileges for Spork so I’m not sure what its settings are but in the repos I run, I have the security setting in Settings > Actions set to ‘Require approval for first time contributors’. If that’s similarly set with Spork and it’s considered overly limiting, @bakpakin could set it to ‘Require approval for first-time contributors who are new to GitHub’.

My view is that requiring approval for first-time contributors is a reasonable default and not an excessive amount of friction. In other words, I would revert the change in 05eb014.

rwtolbert · 2025-08-15T22:31:20Z

@pyrmont Thanks for the follow-up.

I feel bad since my first PR looked good (and wasn't) and the merge has broken janet-pm on master.

pyrmont · 2025-08-15T22:36:22Z

I don't think you need to feel bad. I assume the reason the checks that are causing a problem now because they are running against the current HEAD is the same reason that the checks in that PR didn't catch the error. Just a mistake in the workflow file!

rwtolbert · 2025-08-15T22:42:44Z

Very true. and I appreciate the sentiment. But it drives me crazy to have broken code - especially when I've done the breaking.

And from what I can tell, there is no way I can get this to work from my end.

rwtolbert · 2025-08-15T23:30:13Z

and perhaps this is currently moot, since the tests don't run on Windows. I just got access to my Windows machine and I'm working through the sh/exec stuff in the tests.

sogaiu · 2025-08-16T02:44:34Z

I tried ef0f842 on a Linux box.

It looks like jpm test won't work without spork already installed may be?

running test/suite-pm-pre-post.janet ...
executable scripts have been installed to /home/user/src/spork.rwtolbert/tmp/tmp_dir_0894973558081855.tmp/bin
error: could not find module spork:
    /home/user/.local/lib/janet/spork.jimage
    /home/user/.local/lib/janet/spork.janet
    /home/user/.local/lib/janet/spork/init.janet
    /home/user/.local/lib/janet/spork.so
  in require-1 [boot.janet] on line 3126, column 20
  in import* [boot.janet] on line 3170, column 15
  in thunk [/home/user/src/spork.rwtolbert/tmp/tmp_dir_0894973558081855.tmp/bin/bin-no-hardcode] (tail call) on line 3, column 1
error: command failed with non-zero exit code 1
  in os/proc-wait [src/core/os.c] on line 647
  in ev/gather [spork/sh.janet] on line 34, column 19
  in defer [boot.janet] on line 3900, column 13
  in wait-for-fibers [boot.janet] on line 3893, column 5
  in defer [spork/sh.janet] on line 32, column 17
  in exec-slurp [spork/sh.janet] on line 31, column 3
  in test-binscript [test/suite-pm-pre-post.janet] on line 58, column 22
  in defer [test/suite-pm-pre-post.janet] on line 149, column 8
  in thunk [test/suite-pm-pre-post.janet] (tail call) on line 68, column 1
non-zero exit code in test/suite-pm-pre-post.janet: 1

With jpm already installed I get "All tests passed." though :)

With SPORK_TEST_ALL_PACKAGES=1 jpm test there looks to be some issue (only selected output below):

From https://github.com/pyrmont/testament
 * [new branch]      master     -> origin/master
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
From https://github.com/pyrmont/testament
 * branch            HEAD       -> FETCH_HEAD
HEAD is now at a434abe Support dynamic bindings :tests and :skips to call/skip tests
error: assert failure in (dyn *install-manifest*)
  in pre-install [/tmp/tmp_dir_831599496785441.tmp/spork/declare-cc.janet] on line 363, column 25
  in edefer [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] on line 118, column 22
  in _while [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] on line 112, column 7
  in worker [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] on line 105, column 5
  in defer [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] on line 19, column 11
  in wait-for-fibers [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] on line 12, column 3
  in run-rules [/tmp/tmp_dir_831599496785441.tmp/spork/build-rules.janet] (tail call) on line 132, column 3
  in install [/tmp/tmp_dir_831599496785441.tmp/spork/declare-cc.janet] (tail call) on line 317, column 5
  in do-hook [boot.janet] on line 4210, column 5
  in edefer [boot.janet] on line 4358, column 7
  in bundle/install [boot.janet] (tail call) on line 4339, column 5
  in pm-install [spork/pm.janet] on line 313, column 5
  in pm-install [spork/pm.janet] on line 300, column 7
  in pm-install [spork/pm.janet] on line 300, column 7
  in defer [test/suite-pmfull.janet] on line 42, column 5
  in thunk [test/suite-pmfull.janet] (tail call) on line 33, column 3
non-zero exit code in test/suite-pmfull.janet: 1

bakpakin · 2025-08-16T02:48:54Z

Thanks for all the effort here, @rwtolbert . In the meantime, I am reverting the last merge to get builds working again. I haven't taken a closer look but may find some time this weekend to see if I can help. There does seem to be some CI weirdness going on, which should separately be investigated and improved.

sogaiu · 2025-08-16T03:15:01Z

Did some testing on Linux for b257381 (has had two "reverts") and what I think is the equivalent bcad982.

Good news: jpm test works without installation in both.

Other news: SPORK_TEST_ALL_PACKAGES=1 jpm test seems to be having some kind of issue in both (output truncated below):

remote: Enumerating objects: 161, done.
remote: Counting objects: 100% (30/30), done.
remote: Compressing objects: 100% (24/24), done.
remote: Total 161 (delta 10), reused 8 (delta 5), pack-reused 131 (from 1)
Receiving objects: 100% (161/161), 3.77 MiB | 10.19 MiB/s, done.
Resolving deltas: 100% (88/88), done.
From https://github.com/janet-lang/sqlite3
 * [new branch]      master     -> origin/master
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
From https://github.com/janet-lang/sqlite3
 * branch            HEAD       -> FETCH_HEAD
HEAD is now at a19ec48 Merge pull request #20 from talmeme/using_extensions
error: project.janet:37:1: compile error: unknown symbol body
  in dofile [boot.janet] on line 3102, column 7
  in thunk [/tmp/tmp_dir_899849061149376.tmp/bundle/sqlite3/init.janet] (tail call) on line 4, column 1
  in dofile [boot.janet] (tail call) on line 3101, column 7
  in source-loader [boot.janet] on line 3113, column 15
  in require-1 [boot.janet] (tail call) on line 3137, column 18
  in defer [boot.janet] on line 4200, column 7
  in get-bundle-module [boot.janet] on line 4192, column 5
  in edefer [boot.janet] on line 4351, column 19
  in bundle/install [boot.janet] (tail call) on line 4339, column 5
  in pm-install [spork/pm.janet] on line 313, column 5
  in pm-install [spork/pm.janet] on line 300, column 7
  in defer [test/suite-pmfull.janet] on line 42, column 5
  in thunk [test/suite-pmfull.janet] (tail call) on line 33, column 3
non-zero exit code in test/suite-pmfull.janet: 1

rwtolbert · 2025-08-16T12:11:55Z

thanks for all the eyes on this. If the CI/CD changes happen, I'll try again.

pyrmont · 2025-08-16T12:45:01Z

@rwtolbert The CI change was rolled back in b257381 earlier. It should be safe to submit PRs again (although maintainer approval might be required at first depending on the setting).

rwtolbert · 2025-08-16T22:05:41Z

Submitted again in #230 after updates to CI/CD.

rwtolbert added 2 commits August 14, 2025 08:04

empty change to bump actions to rebuild

8e3048d

add local assertf for older Janet

6e1c889

rwtolbert added 2 commits August 15, 2025 13:00

fix problem in janet-pm where hardcoded syspath at the top-level is n…

1e710db

…ot used when run as main. This change adds a copy of the hardcoded paths inside a main in a binscript, if main exists

Merge branch 'fix_empty_rules' of github.com:rwtolbert/spork into fix…

f7d16f3

…_empty_rules

add extra file extension for running bat file binscripts on Windows

ef0f842

sogaiu mentioned this pull request Aug 16, 2025

CI/CD feature: Remove "This workflow requires approval from a maintainer" #178

Merged

rwtolbert closed this Aug 16, 2025

sogaiu mentioned this pull request Aug 17, 2025

Try 2: add more pre/post steps for janet-pm #230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Brief: Add empty rules for pre-clean/post-clean #229

Brief: Add empty rules for pre-clean/post-clean #229

Uh oh!

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 15, 2025 •

edited

Loading

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

pyrmont commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

pyrmont commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

sogaiu commented Aug 16, 2025 •

edited

Loading

Uh oh!

bakpakin commented Aug 16, 2025

Uh oh!

sogaiu commented Aug 16, 2025 •

edited

Loading

Uh oh!

rwtolbert commented Aug 16, 2025

Uh oh!

pyrmont commented Aug 16, 2025

Uh oh!

rwtolbert commented Aug 16, 2025

Uh oh!

Uh oh!

Brief: Add empty rules for pre-clean/post-clean #229

Brief: Add empty rules for pre-clean/post-clean #229

Uh oh!

Conversation

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 14, 2025

Uh oh!

rwtolbert commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

pyrmont commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

pyrmont commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

rwtolbert commented Aug 15, 2025

Uh oh!

sogaiu commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bakpakin commented Aug 16, 2025

Uh oh!

sogaiu commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwtolbert commented Aug 16, 2025

Uh oh!

pyrmont commented Aug 16, 2025

Uh oh!

rwtolbert commented Aug 16, 2025

Uh oh!

Uh oh!

rwtolbert commented Aug 15, 2025 •

edited

Loading

sogaiu commented Aug 16, 2025 •

edited

Loading

sogaiu commented Aug 16, 2025 •

edited

Loading