-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an (inline ...) stanza #371
Conversation
doc/jbuild.rst
Outdated
ignored. However, when building the ``jbuild`` alias, jbuilder will | ||
run ``<action>`` and make sure that the output of the action matches | ||
``<stanzas>``. If not, jbuilder will update the jbuild file in place | ||
and print a diff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some details for the doc:
- Does
jbuilder
fail/succeed in that case? - How could one easily check that all the
<stanzas>
are up to date (during CI for example). ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails. Jbuilder needs to be restarted to pickup the changes.
You just need to build the @jbuild
alias to make sure that all the <stanzas>
are up-to-date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the doc
What about using a plugin to address the intended use case with camomile? While this feature also addresses that use case, I'm not sure if it's the best option here as having all this boilerplate in your jbuild file is also kind of ugly. |
Both use cases are valid I think. In general let's consider the following scenario:
If you go the plugin way, you have to copy the library for parsing the file inside the plugin, implement the plugin including choose a syntax for it, debug it, etc...
In general, I expect that plugins will be better suited for more general purpose features, such as ctypes. An alternative would be to support |
OK, I'm convinced. I still have a couple of minor points regarding usability:
Anyway, this is cool 👍 |
Yh, I was wondering about the syntax. Putting the generated stanzas inside the The
This is useful when you have a generator that generate static code and want to review the generated code. Moreover, coupled with an option to disable promotion, this gives you a way to cut some dependencies for the release. So in the end you would write that: (rule
((promote)
(action (with-stdout-to jbuild.generated (run ./gen.exe)))))
(include jbuild.generated) I guess this makes this PR obsolete. |
Yes! I can already see this in a few places where an ad-hoc approach is taken to version controlling generated code.
One last thing regarding this. Can this perhaps be generalized into some command that just accepts .correction files? I could see how such a command could be useful for linting and ppx_expect as well. Though it might require a bit more design to make it work nicely in these cases. |
What do you have in mind? |
Just have build generate jbuild.generated.corrected files in _build/ after failing and a simple |
I see, yh that does makes sense. And it moves the burden of printing the diff, choosing the diff command, etc... from the individual systems to only the build system. I like that. To be sure we are on the same page, what the user will write is: (rule (with-stdout-to jbuild.generated.corrected (run ./gen.exe)))
(include jbuild.generated) And jbuilder will do something special because the target ends in |
Yes, that is what I meant. But I did not imagine this part:
I still assumed people would have to mark the tests as inline manually. Though this is acceptable as well. |
I agree with @rgrinberg . I prefer to have something explicit, for understandability. For example exactly what you propose, but just add a stanza for the link:
I didn't follow with which default you en up with. |
Ok to make it explicit. I think it should be used by default and we can disable it in release mode. In fact there are three possible choices:
Basically, if we write this: (promote a as b) then whenever the build needs (executable
((name foo)
(modules (foo bar))))
(rule (with-stdout-to bar.ml.gen (run ./foo.exe)))
(promote bar.ml.gen as bar.ml) For the jbuild case, we'll write this: (rule (with-stdout-to jbuild.inc.gen (run ./gen.exe)))
(promote jbuild.inc.gen as jbuild.inc)
(include jbuild.inc) and Implementation-wise, we can add an action |
What is the release mode? We already have the dev mode, I think it should be checked for equality only in dev mode.
+1 |
May I just raise the point that the current hard-coded formulation of |
It's when you pass
Absolutely, in the future, I'd like to add profiles. We'll have the Profiles should be selectable from the command line or from the (set_defaults
(dev ((flags (:standard -w +a)))
(coverage ((preprocess (pps (bisect.ppx)))) |
Adding profiles sounds excellent! I imagine that it would allow using sexp jbuild files in cases like this. |
Indeed, it will. I did notice this jbuild file yesterday when I was trying a patched compiler on a big self-contained workspace, I got |
That happens when the |
Ah I see. The general idea is that by dropping multiple jbuilder projects in any layout you get a new bigger project, that we call a workspace in jbuilder. The root of the workspace is marked by a file Ideally individual jbuilder projects shouldn't rely too much on specific jbuilder invocation. BTW, I did a small video tutorial to explain the basics of jbuilder: https://www.youtube.com/watch?v=BNZhmMAJarw. I'm planning to do a second video to explain the packaging part. |
BTW, I agree that the current project layout is not clear. I guess it's because of the way jbuilder was developed. When we rename jbuilder, we'll make it clearer: projects will be expected to have a jbuild file (or whatever the new name will be for these files) at the root of their project containing a |
Closing this as this is superseded by #402. The only thing missing the the toplevel promote stanza. To implement it we'll need to add a way to extend the curren goal,. |
This feature adds an
(inline ...)
stanza whose purpose is to provide similar functionalities to(include ...)
, except that the inlcuded contents is written directly in the jbuild file. Jbuilder make sure that the inlined contents is kept up-to-date. This is how it works:This is used in
doc/jbuild
. It should cover many cases when one has a long list of repeated rules. Compared to a classic(include ...)
stanza, we have to commit the generated rules, but on the other hand it's easier to debug as we see the generated rules directly in the jbuild file.This feature is intended for cases such as camomile, where we have a lot of rules generated, but the pattern is very specific to one use case, so it doesn't justify writing a plugin and having the actual rules under the eyes makes debugging easier.