mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.33k stars 1.53k forks source link

modules: Add an 'hotdoc' module #4016

Closed thiblahute closed 5 years ago

thiblahute commented 5 years ago

hotdoc: http://github.com/hotdoc/hotdoc/

ignatenkobrain commented 5 years ago

Flake8 detected 6 issues on b4597d099dd2330137b503cdea4ee45f35378687 Visit https://sider.review/gh/19784232/pull_requests/4016 to review the issues.

thiblahute commented 5 years ago

@jpakkane Could you please rebuild your CI docker image with the changes from that PR ?

jpakkane commented 5 years ago

I have started a rebuild of the CI image. It should be uploaded in about an hour. In the future please don't fiddle with the travis files, but change the master Dockerfile under ciimage instead.

thiblahute commented 5 years ago

The AppVeyor failure looks weird and is totally unrelated I believe.

MathieuDuponchelle commented 5 years ago

@thiblahute , you will also need to add documentation, and a release snippet

thiblahute commented 5 years ago

Added documentation and made the small refactoring @MathieuDuponchelle asked for.

MathieuDuponchelle commented 5 years ago

We obviously will need @jpakkane 's approval, but this looks good to go to me now :)

jpakkane commented 5 years ago

The sources argument seems to only handle strings. Should it work with files objects as well? I would imagine people wanting to do something like:

sources = files(...)
shared_library('something', sources)
hotdoc.generate_doc(...
  c_sources : files,
  ...
  )

To only have to list the files once?

thiblahute commented 5 years ago

The sources argument seems to only handle strings. Should it work with files objects as well? I would imagine people wanting to do something like:

Indeed, fixed.

jpakkane commented 5 years ago

Strings should be converted to files always using the interpreter.source_strings_to_files. It does all the necessary magic such as validating that the files in question exist. This should be called on the arglist and then the remaining code only has to deal with File objects.

Some of the arguments such as c_smart_index don't seem to be documented.

And on a completely unrelated tangent: could it be possible to implement this in the opposite way, just like how IDE integration works? That is, if we exposed all the necessary data through the introspection interface, could Hotdoc do everything it needs just with that? This would mean that using Hotdoc (or any documentation system for that matter) could just be a custom target that calls ['hotdoc', '--use-meson-introspection', 'path_to_build_dir_or_something']? It would have the extra benefit that you could change Hotdoc as much as you want without needing to submit anything to Meson core?

thiblahute commented 5 years ago

Strings should be converted to files always using the interpreter.source_strings_to_files. It does all the necessary magic such as validating that the files in question exist. This should be called on the arglist and then the remaining code only has to deal with File objects.

This is not what I need as I am passing file paths to hotdoc to generate the config, and the final target simply generates that target. Meaning that I need the files as paths, the only place where I need a File is to reference the hotdoc config file, which I am doing already

Some of the arguments such as c_smart_index don't seem to be documented.

As explained in the documentation, this is all documented in hotdoc itself using hotdoc help, this is the exact same thing as generate_gir.

And on a completely unrelated tangent: could it be possible to implement this in the opposite way, just like how IDE integration works? That is, if we exposed all the necessary data through the introspection interface, could Hotdoc do everything it needs just with that? This would mean that using Hotdoc (or any documentation system for that matter) could just be a custom target that calls ['hotdoc', '--use-meson-introspection', 'path_to_build_dir_or_something']? It would have the extra benefit that you could change Hotdoc as much as you want without needing to submit anything to Meson core?

Basically hotdoc is a documentation compiler, asking for the it to know the build system would be very weird and would reverse the way the two tools should work together, I do not think it is a good idea. Moreover I started working a another branch where I will basically allow people to simple do

lib = library(....) # Could be a GirTarget also
hotdoc = import('hotdoc')
hotdoc.generate_doc(lib)

and the hotdoc module will do everything required to document the lib in the simplest way possible.

MathieuDuponchelle commented 5 years ago

I concur with thibault here, IMO the logical hierarchy is for IDEs to know about build systems, and for build systems to know about compilers, and I would argue that documentation tools are equivalent to compilers, and should not hold knowledge of the build systems that may call on them.

jpakkane commented 5 years ago

This is not what I need as I am passing file paths to hotdoc to generate the config, and the final target simply generates that target.

Yes, but suppose the user has a typo in the file name. In that case they would get an error much later from hotdoc for missing files. If you use Files for that the error message would be obvious and be printed immediately when the user tries to create a broken file reference. This also makes it simpler for you because you can then depend on the fact that all file objects you get are guaranteed to exist (in case that matters, sometimes it does not).

thiblahute commented 5 years ago

I ended up checking files and directory exist myself partly because of #4094 and because I needed to check folders too.

thiblahute commented 5 years ago

Thanks :-)

marc-h38 commented 4 years ago

Hi Thibault. It's not very intuitive to have to invoke ninja upload to merely rebuild the documentation (and not upload anything).

It's also strange that a bare ninja builds the documentation but only after a ninja clean... and without colored output?!? Weird.

I'd like to make this better but right now I'm completely lost in these inconsistencies. Care to shed some light?

PS: all the above observed with today's master branch at commit be9bff81a2ad

thiblahute commented 4 years ago

It's not very intuitive to have to invoke ninja upload to merely rebuild the documentation (and not upload anything).

ninja upload? Never heard of that! :-)

It's also strange that a bare ninja builds the documentation but only after a ninja clean... and without colored output?!? Weird.

It doesn't build the doc by default unless you specify it when you declare the target, it is build only when installing or when invoking the ninja targetname-doc.

marc-h38 commented 4 years ago

Hi Thibault, I think I figured out why we were talking across each other. I should have written that I'm very specifically referring to building the documentation of meson itself and your commit https://github.com/mesonbuild/meson/pull/4016/commits/6f72473b2457ecd4, and especially this line of yours:

ninja -C build/ upload
thiblahute commented 4 years ago

Hi Thibault, I think I figured out why we were talking across each other. I should have written that I'm very specifically referring to building the documentation of meson itself and your commit 6f72473, and especially this line of yours:

ninja -C build/ upload

Ah, I just ported previous doc there (where it was explaining how to upload the doc), you can just run ninja -C build/ in there as the hotdoc target is build_by_default: true

marc-h38 commented 4 years ago

ninja -C build works but only the first time. ninja -C build upload seems to rebuild every time; while not ideal it's much better.

$ touch markdown/*
$ ninja -C build/
ninja: Entering directory `build/'
ninja: no work to do.
$ ninja -C build/ upload
ninja: Entering directory `build/'
[0/1] Running external command upload.
WARNING: [core]: (markdown-bad-link): ...
MathieuDuponchelle commented 4 years ago

ninja -C build clean && ninja -C build, while not ideal, should do the trick. hotdoc has support for generating deps files:

  --deps-file-dest DEPS_FILE_DEST
                        Where to output the dependencies file
  --deps-file-target DEPS_FILE_TARGET
                        Name of the dependencies target

but I don't think this is used by the hotdoc module.

thiblahute commented 4 years ago

but I don't think this is used by the hotdoc module.

It does and it worked at some point, we should check why it doesn't anymore.

MathieuDuponchelle commented 4 years ago

Oh, good to know, then yeah that would be the thing to check first :)

xclaesse commented 4 years ago

@MathieuDuponchelle @thiblahute : While you're here, would you guys agree to add yourself in CODEOWNERS for the hotdoc module?

thiblahute commented 4 years ago

@MathieuDuponchelle @thiblahute : While you're here, would you guys agree to add yourself in CODEOWNERS for the hotdoc module?

I guess I am fine with that, what does it involve in practice?

xclaesse commented 4 years ago

You get requested review automatically for any PR that touch that module. More generally, you are the maintainer of that module, you take the decisions about new features, etc that gets included. I've been pushing to delegate more responsibilities in the Meson project: https://github.com/mesonbuild/meson/issues/6485.

thiblahute commented 4 years ago

I am fine with that.

MathieuDuponchelle commented 4 years ago

that is a good idea yes :) thib knows more about the module than I do however

marc-h38 commented 4 years ago

ninja -C build clean && ninja -C build, while not ideal

While I have admittedly no idea why it works, ninja -C build upload seems to do the same, is shorter to type and is what @koponomarenko documented in meson/docs/README.md (PR #4275 commit e53be9c925ea - before CODEOWNERS I guess :-)

hotdoc has support for generating deps files:

Depending on the complexity this may be overkill for meson/docs/ which take less than 2 seconds to generate.

MathieuDuponchelle commented 4 years ago

While I have admittedly no idea why it works, ninja -C build upload seems to do the same

Sure, but if the goal is to simply rebuild the local copy, then there's not much point in trying to upload to a place where you probably don't have write access :D

Depending on the complexity this may be overkill for meson/docs/ which take less than 2 seconds to generate.

Right, but meson needs some trigger to make it rebuild when some dep has changed doesn't it?

marc-h38 commented 4 years ago

then there's not much point in trying to upload to a place where you probably don't have write access :D

Let me quote the very first sentence I used to start this recent discussion:

Hi Thibault. It's not very intuitive to have to invoke ninja upload to merely rebuild the documentation (and not upload anything).

Back to you:

Right, but meson needs some trigger to make it rebuild when some dep has changed doesn't it?

I meant: no, in this very particular case it could just rebuild every time because it's quick. Like ninja upload already does (how?) for some unknown reason.

MathieuDuponchelle commented 4 years ago

Let me quote the very first sentence I used to start this recent discussion:

Hi Thibault. It's not very intuitive to have to invoke ninja upload to merely rebuild the documentation (and not upload anything).

Yes? My point is that ninja upload isn't the correct command to use to rebuild the doc locally, I gave you the correct one and you replied something about conciseness, I don't really get your point now

I meant: no, in this very particular case it could just rebuild every time because it's quick. Like ninja upload already does (how?) for some unknown reason.

ninja upload presumably uses hotdoc's uploading feature, and hotdoc always rebuilds when called. Re rebuilding every time, that's hardly the desirable outcome, a well-behaved project should do nothing on the second of two successive calls to ninja -C build, I'm sure we can agree on that. Repeating what I said earlier, we should look into why the depsfile setup is broken, it's probably not very difficult to fix.

thiblahute commented 4 years ago

OK, here is the fix in meson: https://github.com/hotdoc/hotdoc/pull/184

Although I add this issue with ninja 1.9 which is fixed in ninja 1.10.

marc-h38 commented 4 years ago

My point is that ninja upload isn't the correct command to use to rebuild the doc locally, I gave you the correct one and you replied something about conciseness, I don't really get your point now

Involving ninja clean; ninja every time is neither convenient nor the "correct" way to build anything.

I will try the fix, thanks Thibault.

MathieuDuponchelle commented 4 years ago

Involving ninja clean; ninja every time is neither convenient nor the "correct" way to build anything.

It's the correct way if you want to rebuild from scratch, using the upload target as a workaround clearly isn't, anyway this argument is pointless

marc-h38 commented 4 years ago

It [ninja clean] is the correct way if you want to rebuild from scratch,

Agreed, keeping in mind rebuilding from scratch is a workaround in the first place.

Unless changes are made to the meson.build or related files themselves and then ninja clean is regularly not enough - I wasted way too much time trusting ninja clean in that case so I simply stopped using it when editing meson.build. rm -rf build or git clean -fdx are then the only way. Example: https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/pristine.cmake

See also "meson + ninja wrapper script" comment here: https://github.com/mesonbuild/meson/issues/6434#issuecomment-572290358

tl;dr: using ninja clean is always the sign something somewhere is wrong.

anyway this argument is pointless

Agreed, as in: " is bad better than very bad?"

MathieuDuponchelle commented 4 years ago

" is bad better than very bad?"

Good news is with thib's fix the behaviour is correct again so we can forget about the argument :D

eli-schwartz commented 4 years ago

Unless changes are made to the meson.build or related files themselves and then ninja clean is regularly not enough - I wasted way too much time trusting ninja clean in that case so I simply stopped using it when editing meson.build. rm -rf build or git clean -fdx are then the only way.

If meson's builtin support for reconfiguring on changes to meson.build isn't enough for you (why not???) then I would recommend using meson setup --wipe build/ instead, which should be the same as rm -rf build/ except it preserves the command line you previously used. ;)

marc-h38 commented 4 years ago

Good news is with thib's fix the behaviour is correct again

I thought I was trying the fix yesterday but realized at last the fix is in hotdoc itself and not in the meson module and that the fixed hotdoc version isn't released in pip yet :-/ Will try again another time.

If meson's builtin support for reconfiguring on changes to meson.build isn't enough for you (why not???)

It is enough... most but not all the time. When changing cross files maybe? I'm actually amazed it is enough most of the time, however my project is small enough that it's just faster to rm -rf build/ and not think about edge cases.

I would recommend using meson setup --wipe build/ instead

I tried --wipe and I'd love to use it, however it fails when build/ doesn't exist so rm -rf build is just simpler to script. Already reported in https://github.com/mesonbuild/meson/issues/6434#issuecomment-572290358