Skip to content

Make lessc mandatory#10239

Closed
jcarrano wants to merge 2 commits intoRIOT-OS:masterfrom
jcarrano:make-lessc-mandatory
Closed

Make lessc mandatory#10239
jcarrano wants to merge 2 commits intoRIOT-OS:masterfrom
jcarrano:make-lessc-mandatory

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Oct 24, 2018

Contribution description

The riot.css can be generated by the lessc tool from riot.less, but it is also tracked by git. If the user dows not have the tool, the docs makefile automatically decides not to rebuild it.

The two options to fix this are:

  1. Remove the rule to make riot.css, make that step manual.
  2. Remove the file and make the step for creating it required.

Since lesscpy (the python version of the tool) is super-easy to install this commit implements (2). This also ensures that everyone always sees the same version of riot.css.

Testing procedure

Install lesscpy from PIP and run make docs. The docs should be built fine.

Issues/PRs references

Depends on #10237 so that travis does not fail.
I don't know if Murdock has lesscpy.

Replaced by #10249

The version of lesscpy in Ubuntu 14 (in travis) is super outdated and
causes the make docs to fail.
The riot.css can be generated by the lessc tool from riot.less, but
it is also tracked by git. If the user dows not have the tool, the
docs makefile automatically decides not to rebuild it.

The two options to fix this are:

1. Remove the rule to make riot.css, make that step manual.
2. Remove the file and make the step for creating it required.

Since lesscpy (the python version of the tool) is super-easy to install
this commit implements (2). This also ensures that everyone always sees
the same version of riot.css.
@jcarrano jcarrano requested review from aabadie and cladmi October 24, 2018 13:03
@jcarrano jcarrano added State: waiting for other PR State: The PR requires another PR to be merged first Area: doc Area: Documentation labels Oct 24, 2018
@miri64
Copy link
Member

miri64 commented Oct 24, 2018

Why can't we keep the job optional? Making it manual runs the risk, that the user forgets to update CSS, making it required only add to the heap of tools required to install for any user that wants just to check if their doc is building properly.

@jcarrano
Copy link
Contributor Author

The problem with optional is that in this case it means "optional without user control".

If for any reason I have the less compiler installed on my machine it will be used to update the css and it would have no option but to manually revert the changes in the css each time, or uninstall lessc, or manually touch the css to trick make. All of which are hacky solutions.

Making it manual is preferable to that.

@miri64
Copy link
Member

miri64 commented Oct 24, 2018

If for any reason I have the less compiler installed on my machine it will be used to update the css and it would have no option but to manually revert the changes in the css each time, or uninstall lessc, or manually touch the css to trick make. All of which are hacky solutions.

Ideally, your resulting CSS should be the same as the existing ones. Another non-hacky solution would be to just add the riot.css to .git/info/exclude. This way it will not show up everytime you do make doc (but you would need to git add -f if and only if you changed something in riot.less, which rarely happens ;-)).

Making it manual is preferable to that.

I don't think so. Forcing users to install tools they don't need (this is just about the looks of the generated HTML, mind you), seems unhelpful.

@jcarrano
Copy link
Contributor Author

Ideally, your resulting CSS should be the same as the existing ones.

Sure, but there can be minor differences (check it out, it's mostly whitespace).

Making it manual is preferable to that.

I don't think so. Forcing users to install tools they don't need (this is just about the looks of the generated HTML, mind you), seems unhelpful.

When I say "make it manual" I mean keep the css in git, but remove the rule to remake it from the makefile (or replace it with a .FORCE rule with a non-file name). This way the CSS is rebuilt only if requested by the user.

Regardless of what we choose, I want to avoid automagical behaviour. This is what was causing travis to NEVER check the docs (because travis was trying to rebuild the CSS, and that is something that normally does not happen on the user's PC).

@miri64
Copy link
Member

miri64 commented Oct 24, 2018

Regardless of what we choose, I want to avoid automagical behaviour. This is what was causing travis to NEVER check the docs (because travis was trying to rebuild the CSS, and that is something that normally does not happen on the user's PC).

Murdock is and will stay for the forseeable future be our canonical CI, so I don't see the problem. Every additional tool that is required just to build something is a burden to the user. Make one required just so one misconfigured CI is happy, shouldn't be the way to go.

@jcarrano
Copy link
Contributor Author

Make one required just so one misconfigured CI is happy, shouldn't be the way to go.

Totally agree, that's why I also propose to drop the rule to create the CSS. The real issue is not even Travis. The issue is the the user types make docs and maybe- depending on what's on his computer - a file tracked by git gets changed.

My proposal in this PR was "riot.css always gets changed and we don't track it anymore" but I can go back and do another PR with "riot.css is tracked and does not get updated unless one explicitly types make stylesheet".

@miri64
Copy link
Member

miri64 commented Oct 24, 2018

My proposal in this PR was "riot.css always gets changed and we don't track it anymore" but I can go back and do another PR with "riot.css is tracked and does not get updated unless one explicitly types make stylesheet".

👍 I think that's a good compromise (I'd prefer doc-css as a target name though, makes it clearer what it actually does ;-))

@jnohlgard
Copy link
Member

👍 for separating the targets, 👎 to requiring lesscpy for building the doxygen docs.

@miri64 miri64 added the Discussion: contested The item of discussion was contested label Oct 25, 2018
@jcarrano
Copy link
Contributor Author

I'm closing this in favor of #10249.

@jcarrano jcarrano closed this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation Discussion: contested The item of discussion was contested State: waiting for other PR State: The PR requires another PR to be merged first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants