Skip to content
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 -MF -MP and -MT in line with gcc dependency tracking. #3087

Closed
wants to merge 2 commits into from
Closed

Add -MF -MP and -MT in line with gcc dependency tracking. #3087

wants to merge 2 commits into from

Conversation

de-vri-es
Copy link

@de-vri-es de-vri-es commented Jul 20, 2017

This PR does a few things:

  • Make the change in make --depends generate no CSS output #2830 optional by adding a --no-css flag. By default --depends now outputs CSS again. This flag also has the benefit that you can now use lessc as a syntax/import checker without generating any output (css or dependencies).
  • Add the main source file as a dependency for the output file (feature request: input source file with --depends options #2476) in the generated dependency information.
  • Add -MP --depends-phony, -MT --depends-target, -MF --depends-file in line with gcc flags (all of these imply -M). These flags allow much more efficient integration with make.

-MP or --depends-phony adds phony rules for all dependencies. This prevents make from generating errors when a dependency is deleted.

Someone also wrote a blog article on post-processing the dependency information to achieve the same result using a pipe of sed, tr and sed [1]. Personally I've been using an awk script so far. I hope that shows that there is actual demand for this feature.

[1] https://www.thumbtack.com/engineering/makefiles-for-less-and-css/

Example output:

$ lessc /tmp/test.less /tmp/test.css  -MP 
/tmp/test.css: /tmp/test.less /tmp/foo.less
/tmp/foo.less:

-MT or --depends-target overwrite the target of the main rule in the dependency file. This can be useful when the output of lessc is later moved, or if the generated CSS output goes to stdout. Example output:

$ lessc /tmp/test.less /tmp/test.css  -MT=foo.css
foo.css: /tmp/test.less /tmp/foo.less
/tmp/foo.less:

-MF or --depends-file writes the dependecy information to a file instead of stdout. Together with -MT this enables dependency tracking even when CSS output goes to stdout.

@de-vri-es
Copy link
Author

de-vri-es commented Jul 20, 2017

Hmm, did I cause that test failure? I get the same failure locally but it's not clear to me how I could have caused it.

/edit: Also, maybe --no-write should be called --no-css ? (renamed it)

@seven-phases-max
Copy link
Member

seven-phases-max commented Jul 20, 2017

Add -MP --depends-phony, -MT --depends-target, -MF --depends-file

Oh, no, Please no more options... Who's gonna to maintain them? It's already plenty of options so that even basic issues take forever to clarify and test (this is not GCC with its zillion active contributors).

In it's current state I don't think Less can afford "try to cover all of this" approach. So simple revert of #2830 would be enough.

Hmm, did I cause that test failure?

No, it's the v3 codebase itself (it needs a lot of polishing and clean-up yet, specifically its various build and test stuff).

@de-vri-es
Copy link
Author

de-vri-es commented Jul 20, 2017

Add -MP --depends-phony, -MT --depends-target, -MF --depends-file

Oh, no, Please no more options... Who's gonna to maintain them? It's already plenty of them so that even basic issues take forever to clarify and test (this is not GCC with its zillion active contributors).

I found it remarkably simple to implement these options. I don't think they add a big maintenance burden. All the dependency tracking related options are confined to two functions: logDependencies and writeDependencies, and a check at the start to make sure that we recognize missing arguments before rendering the CSS.

In it's current state --depends is not very suitable for make. Especially the lack of phony targets is a problem, but forcing output to stdout is also not very nice (and we need both -MT and -MF to prevent that).

Telling users to post process the output with sed, tr, or awk is pushing the problem onto users who will inevitably make much more shaky solutions.

And thanks to gcc these flags have a solid meaning, so there shouldn't be any confusion on their intended purpose either.

@matthew-dean
Copy link
Member

I think a "make" build of Less with make-specific options would be better suited for an independent repo that imports the Less engine as a dependency. The purpose of lessc is not to plug into a build toolchain. lessc itself imports the less-node library, which is the same thing that any other tool could do. It's not meant to be a swiss-army knife of build options. Ideally, as @seven-phases-max alludes to, the options would be reduced to lessen the learning curve.

The additional benefit of importing Less.js as a dependency is that your tool can maintain its own documentation. And the tool can still go onto the list of Less-related tools on the lesscss.org site.

@de-vri-es
Copy link
Author

de-vri-es commented Jul 21, 2017

The purpose of lessc is not to plug into a build toolchain. lessc itself imports the less-node library, which is the same thing that any other tool could do.

Then what is it's purpose? As I see it, lessc is the command line interface to less-node. From a user perspective a swiss army knife is exactly what you want there.

As for the complexity of the options:
--no-css is implemented as:

        if (options.noCss) {
            onSuccess();
        } else { ....

-MT is implemented as:

 var target = options.dependsTarget || output;

-MD is implemented as:

        if (options.dependsPhony) {
            for (var i = 0; i < dependencies.length; i++) {
                stream.write(dependencies[i]);
                stream.write(':\n');
            }
        }

Only -MF is slightly too big to simply quote here, but even then it boils down to about 20 extra lines compared to simply writing to stdout (of which 6 could be removed since the onSuccess callback isn't really used).

It feels to me you're really overestimating how much complexity this adds versus the gained benefit for the users.

However, if you really don't like command line options, we could do it slightly differently. We could drop the -MP flag but still output the phony rules, in effect making -MP non-optional. Instead of having an -MF flag we could add an optional argument to the current -M flag. That argument would be the filename where to put the output. Additionally, we can just drop the -MT flag and reduce the flexibility on that point.

Or if even that is to much we could drop all the new flags, don't add optional arguments, but still write the phony rules. Personally, I prefer to have the flags as gcc defines them because they are well known, but the most important thing to me is usable dependency tracking.

As for --no-css, I think it deserves to exist. People obviously want it, as can be seen from #2830. I don't need it, and it should not be implied by -M, but it is so simple to implement and apparently it makes people happy.

@seven-phases-max
Copy link
Member

seven-phases-max commented Jul 21, 2017

It's does not matter how many code a new option require... It's just 2 options -> 2^2 = 4 behaviour variants, 5 options -> 2^5 = 32 and so on... (Not counting documentation etc.).

Sometimes they are unavoidable (you don't have too much choice if you must do /arm or /sparc), sometimes they aren't. I would immediately throw in 1000 new cool options if I wouldn't know there's nobody to fix it it's got screwed.

@de-vri-es
Copy link
Author

de-vri-es commented Jul 21, 2017

I do think that the risk of breakage is low because their usage is confined to dependency generation. I could even wrap it in a way that all the code is neatly together in a separate object. Additionally, I'm willing to jump in if they do cause problems. I can be tagged on github and my email address is publicly available on my github profile.

But like I said, my main goal is to improve dependency tracking for make. So if you will not accept those flags, what about my proposal of dropping the flags but always adding phony rules? That, together with reverting #2830 will still significantly improve make and lessc interaction.

That leaves the question if --no-css should be kept to keep #2830 happy.

@stale
Copy link

stale bot commented Nov 18, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 18, 2017
@de-vri-es de-vri-es closed this Nov 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants