Skip to content

Conversation

@pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Sep 23, 2025

This PR updates the bundle/ functions. It does four things:

  1. tidies up docstrings
  2. changes a hook name from :dependencies to :postdeps
  3. changes :has-bin-script to :has-bin
  4. add a bundle/add-manpage function

The changes are in four separate commits so that they can be cherry-picked if one or more of them are undesirable.

I think the changes are mostly self-explanatory but the reason to change :has-bin-script to :has-bin is because the file that is added to the binpath is not necessarily a binscript. Or at least, I can’t see any reason why it is necessarily a binscript (in Predoc, I add a file using bundle/add-bin and it is a compiled file).

@sogaiu
Copy link
Contributor

sogaiu commented Sep 23, 2025

I think syspath is a thing, but perhaps binpath and manpath don't necessarily refer to "things" in the same way. That is, there don't appear to be any established corresponding dynamic variables, unlike (the name of) (dyn *syspath*) for syspath.

In the context of reading docstrings without source (e.g. on the core api page where there is no source code visible), I think it might be better to not say binpath or manpath.


A perhaps less relevant remark concerns the use of / within a docstring, e.g.

(string (dyn *syspath*) "/bin") 

Although Windows might allow use of / as a file path separator in many contexts, this use within a docstring doesn't seem so good to me.

Possibly instead of text like:

Scripts will be copied to (string (dyn *syspath*) "/bin")

we could say instead something like:

Scripts will be copied to the bin subdirectory of syspath

Side benefit: the suggested alternative actually takes less characters :)

@pyrmont pyrmont force-pushed the feature.bundle-manpages branch from acc6c75 to 1ed9deb Compare September 23, 2025 09:44
@pyrmont
Copy link
Contributor Author

pyrmont commented Sep 23, 2025

I agree with @sogaiu about binpath and manpath. I'm not sure if his wording is the right way to describe it but I couldn't think of a better alternative so I've updated with that. The changes have been force pushed. (I normally prefer to avoid force pushing to PRs but I've done so here to preserve the cherry-picking potential.)

@sogaiu
Copy link
Contributor

sogaiu commented Sep 23, 2025

Re: 3. changes :has-bin-script to :has-bin

I started looking a bit more closely at janet-pm's source and as part of this noticed that both jpm and janet-pm have both declare-bin and declare-binscript.

It seems there is a difference between a bin and a binscript, as the latter supports a :hardcode-syspath construct that seems to lead to tweaking of the script. Incidentally this was touched on briefly in this issue (see point 4):

  1. Fixes existing problem in janet-pm where hardcoded syspath at the top-level is not honored when run as (defn main .... This change adds a copy of the hardcoded paths inside defn main in a binscript, if main exists in the input.

Not sure what the consequences of :has-bin-script are but thought I'd point out the bits above as a factor for consideration 😅

@sogaiu
Copy link
Contributor

sogaiu commented Sep 24, 2025

I've only managed to find a couple of places where :has-bin-script occurs:

The comment from the OP:

the file that is added to the binpath is not necessarily a binscript

seems correct for install-rule, but in the case of install-buffer, that seems to currently be used only for declare-binscript.

@sogaiu
Copy link
Contributor

sogaiu commented Sep 25, 2025

A bit of a tangent...looking at install-buffer and having seen the idea of bundle/add-manpage in this PR, I thought it might be useful to have an bundle/add-buffer function.

I have some bundle scripts that dynamically create a file only to erase it after calling bundle/add-bin...

@sogaiu
Copy link
Contributor

sogaiu commented Sep 26, 2025

Regarding :has-bin-script:

I failed to mention in this comment that the two linked locations are in spork.

May be that means that if there is going to be a change here, it would be better for there to be a coordinated change in spork as well?

@pyrmont
Copy link
Contributor Author

pyrmont commented Sep 26, 2025

@sogaiu The Spork functions set the value of :has-bin-script to true but no functions there seem to use it.

@sogaiu
Copy link
Contributor

sogaiu commented Sep 26, 2025

IIUC, the change in this PR is within a function that checks an associated value of :has-bin-script and performs an action.

May be due to timing or ordering it doesn't make a difference?

It seemed possible that the spork functions might have been expecting to cause a certain behavior in boot.janet. I think in this case that might have been to produce a certain output. If the change in question in this PR is made and the spork side doesn't change then couldn't the behavior end up being different than before? That is, wouldn't there no longer be output when there was before?

Perhaps this is all mistaken though 😅

@pyrmont
Copy link
Contributor Author

pyrmont commented Sep 26, 2025

Oh, sorry, I misunderstood. Yes, those two functions in Spork would need to be updated if this change is accepted. However, also yes that the only thing that would occur if they were not updated is that a message on installation would not be printed out.

@sogaiu
Copy link
Contributor

sogaiu commented Sep 26, 2025

Another thing I noticed is that :has-bin-script and value seems to end up in various manifest files:

$ janet Janet 1.40.0-ed2ae562 linux/x64/gcc - '(doc)' for help repl:1:> (-> (string (dyn *syspath*) "/bundle/jdoc/manifest.jdn") slurp print) @{:dependencies @[] :files @["/home/user/.local/lib/janet/bin/jdoc"] :has-bin-script true :hooks @[:install] :info {:name "jdoc"} :local-source "/home/user/src/jdoc" :name "jdoc"} nil 

Not too many users at this point are likely to have these files and I guess the values are not really used in any other way...so not really a problem?

@pyrmont
Copy link
Contributor Author

pyrmont commented Sep 26, 2025

Yeah, that would be my view. I searched across GitHub just now and the only files with a .janet extension which include the string :has-bin-script are copies of boot.janet and declare-cc.janet.

@bakpakin
Copy link
Member

bakpakin commented Sep 28, 2025

The man page stuff was left out because it seemed weird to commit to man pages as a form of documentation for all operating systems including windows. But I'm not totally opposed to it, but maybe make it clear that we can support other forms of documentation besides man pages?

Oh, sorry, I misunderstood. Yes, those two functions in Spork would need to be updated if this change is accepted.

Then this is not going to happen. Let's just leave the current keyword as :have-bin-script and not change it and cause unnecessary breakage.

@pyrmont
Copy link
Contributor Author

pyrmont commented Sep 28, 2025

Then this is not going to happen. Let's just leave the current keyword as :have-bin-script and not change it and cause unnecessary breakage.

Umm, OK, but what are we supposed to do if we have executable files to install that are not scripts? It seems very odd that you shouldn't use bundle/add-bin. But if you do use that, then the bundle/install message is misleading because the executable might not be a 'script'.

To be clear, I care about this because Predoc is not a binscript. What should I do?

@bakpakin
Copy link
Member

I'm not particularly dogmatic on :have-bin vs :have-bin-script, but I don't want to cause breakage for no functional reason. As far as I'm concerned, the manifest format is mostly an internal detail so even less of a reason to change it.

On unix/posix, a "script" isn't really that different than a "program", the key point is just that they are things you want on your PATH environment variable. On windows there is a bit more difference but not that much. The message is to indicate where executables are being installed too so that a user can find them or update their PATH.

@bakpakin
Copy link
Member

To be clear, I care about this because Predoc is not a binscript. What should I do?

Does bundle/add-bin not work for this?

@bakpakin
Copy link
Member

I have cherry picked all the commits except the change on the manifest format. Thanks for the help here, @pyrmont!

I'm going to close this issue - if there is further reason/discussion for making more distinction between executable scripts and programs, lets have another issue/PR.

@bakpakin bakpakin closed this Sep 29, 2025
@bakpakin bakpakin added the cherry picked Commits from this PR have been cherry picked, so that PR should not be merged. label Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry picked Commits from this PR have been cherry picked, so that PR should not be merged.

3 participants