Create a rules_sass BUILD file generator #75

achew22 · 2019-02-18T21:25:24Z

This change uses Gazelle (https://github.com/bazelbuild/bazel-gazelle),
a BUILD file generator for Bazel projects, to generate rules_sass BUILD
files automatically based on the directory structure of a preexisting
project. This supersedes #66.

Gazelle intelligently manages your BUILD files. It doesn't delete any
rules (unless it is appropriate to delete it), operates off of a cache,
and is fast enough to be used by the Kubernetes project for managing
their BUILD files.

How to use this:

Follow the instructions in the Gazelle documentation
for configuring Gazelle.
In the BUILD file in the root of your project, amend the
list of installed languages to additionally include the target,
which is created in this commit,
"@io_bazel_rules_sass//gazelle:go_default_library",
cd into the directory you would like to generate BUILD files for
bazel run //:gazelle
Enjoy your new BUILD files.

jelbourn

High level: would it make more sense to move the code for all of the gazelle related stuff to its own repository? The addition dwarfs the existing code and I don't believe it needs to be tightly coupled in the same repo.

BUILD.bazel

jelbourn · 2019-02-19T21:56:47Z

BUILD.bazel

+)
+
+# In your project you should define your own Gazelle binary. Follow the
+# instructions at


@alexeagle we generally avoid people having to do things like this in the Angular ecosystem, no?

we don't want our users to compile Go code, no. But we haven't tackled the details of how that works along with Gazelle build rules yet.

I'm not sure how to proceed on this.

I believe users could run bazel run @io_bazel_rules_sass//:gazelle without needing to define their own rule. I'm guessing something in the Angular CLI would do that though?

Users will still need to ensure @bazel_gazelle and its dependencies are declared in WORKSPACE.

Let me know if there would be useful changes in the gazelle or gazelle_binary rules to make this easier.

For the whole npm ecosystem we don't want our users to observe a dependency on go code since they have to download a whole Go sdk. Our approach for other go code has been to ship three binaries to npm. I imagine we'll eventually do the same for BUILD file generation - so you'd get something like bazel run @npm//@bazel/gazelle/bin:gazelle and it would not require any dependencies in WORKSPACE

jelbourn · 2019-02-19T21:57:47Z

examples/nested/dir/BUILD

@@ -4,8 +4,8 @@ load("//sass:sass.bzl", "sass_binary")

 # Import our shared colors and fonts so we can generate a CSS file.
 sass_binary(
-    name = "nested",
-    src = "dir/main.scss",
+    name = "dir",


Why change this name?

I ran gazelle over these directories. If you would like them restored to their former glory because you were using them as tests/examples of functionality, just say so.

jelbourn · 2019-02-19T21:58:27Z

examples/nested/dir/BUILD

-    name = "nested",
-    src = "dir/main.scss",
+    name = "dir",
+    src = "main.scss",


I think the point of this test was was to reference a file one directory deep

jelbourn · 2019-02-19T21:59:09Z

examples/shared/BUILD

-)
-
-# make a :shared target that includes both :colors and :fonts.
-sass_library(


Why delete this?

Gazelle doesn't make a :shared target so I deleted it by hand. Note that if I hadn't deleted it, gazelle would have left it alone and it would still be there. Gazelle only updates rules that it would have created if the directory were empty.

gazelle/config.go

gazelle/BUILD.bazel

gazelle/fileinfo.go

gazelle/fileinfo_test.go

gazelle/parser/ident.go

nex3

I don't understand why this scans numbers, functions, hashes, URLs, or any sigils other than @ and ,. All you care about is finding the string @import outside of a comment, and reading strings after it.

gazelle/BUILD.bazel

gazelle/fileinfo.go

gazelle/parser/comment.go

gazelle/parser/string.go

achew22 · 2019-02-24T23:34:07Z

High level: would it make more sense to move the code for all of the gazelle related stuff to its own repository? The addition dwarfs the existing code and I don't believe it needs to be tightly coupled in the same repo.

It's totally possible to break this out into its own repo. I believe it should be included in this repo. As I wrote in #66,

Why should this be in the rules_sass repo?

Great question! I believe this should live in the rules_sass repo so
that the generator of BUILD files and the macros/rules in the repo can
be iterated on in lock step. This will help prevent circumstances where
a user of rules_sass forgets to update their dependency on Gazelle and
ends up generating garbage BUILD files that are only logically to a
former state of the rules_sass project.

To expand on this, Gazelle is the best story that Bazel has for iterating on rules_* repos. By using your source files as a declarative language for creating BUILD files, you can instruct users to invoke gazelle fix and perform any manipulation to the AST of the BUILD file that you need. If you are going to add a parameter to your BUILD files why not have gazelle fix it for your users when they next run gazelle after updating the SHA to load your repo?

jelbourn · 2019-02-25T21:07:45Z

I talked to @alexeagle and he agreed that ti make sense for the code to be here, so sgtm.

Looks like the CI is failing on the Windows build.

nex3 · 2019-02-26T21:48:01Z

I think this comment is still unaddressed...

jayconrod

I'm excited to see this making progress. A few comments on rule generation, parser structure, and Go readability. Structure generally looks good though. I'm afraid I don't know enough about sass to comment on anything related to the syntax though.

jayconrod · 2019-02-26T21:48:05Z

BUILD.bazel

+)
+
+# In your project you should define your own Gazelle binary. Follow the
+# instructions at


I believe users could run bazel run @io_bazel_rules_sass//:gazelle without needing to define their own rule. I'm guessing something in the Angular CLI would do that though?

Users will still need to ensure @bazel_gazelle and its dependencies are declared in WORKSPACE.

Let me know if there would be useful changes in the gazelle or gazelle_binary rules to make this easier.

jayconrod · 2019-02-26T21:57:56Z

gazelle/config.go

+limitations under the License.
+*/
+
+package gazelle


This is kind of a nit, but consider renaming this to package sass or package sasslang.

For the most part, this would only be imported in generated code, so the package name doesn't matter too much, but in some cases, language extensions do refer to definitions in other extensions. For example, the Go extension refers to information extracted by the proto extension (which is package proto) in order to generate go_proto_library rules.

If you decide to change this, it would probably also mean moving this package to a subdirectory //gazelle/sass so that the package name matches the end of the import path.

jayconrod · 2019-02-26T22:02:16Z

gazelle/fileinfo.go

+
+	file, err := os.Open(filepath.Join(dir, name))
+	if err != nil {
+		log.Printf("%s: error reading sass file: %v", info.Path, err)


Consider returning an error and letting Generate handle the error (by printing it). sassFileInfo may not know what the caller wants to do in case of error.

It's okay to return a partial FileInfo in cases where an error has occurred, but it might be good to distinguish fatal errors (file couldn't be opened) from non-fatal errors (couldn't parse).

It's okay to return a partial FileInfo in cases where an error has occurred, but it might be good to distinguish fatal errors (file couldn't be opened) from non-fatal errors (couldn't parse).

Handled by returning an error in the err value in the fatal case (file not being able to be opened) and adding an "Errors" array to the FileInfo object to represent non-fatal parsing errors.

jayconrod · 2019-02-26T22:10:46Z

gazelle/fileinfo.go

+	parseError := func(msg string, args ...interface{}) {
+		// Prepend the file.Name() so we tell the user which file had the parse error.
+		args = append([]interface{}{file.Name()}, args...)
+		fmt.Fprintf(os.Stderr, "%s: "+msg+"\n", args...)


If you end up returning an error, this function could write to a strings.Builder, then call errors.New with that at the end if it's non-empty.

Resolved with the Errors being first class in the FileInfo object

jayconrod · 2019-02-26T22:16:20Z

gazelle/fileinfo_test.go

+
+			got := sassFileInfo(dir, tc.name)
+
+			// Reexpose the fields we care bout for testing.


Nit: maybe reword to "Clear the fields we don't care about for testing"

jayconrod · 2019-03-01T16:10:27Z

gazelle/sass.go

+// Any non-fatal errors this function encounters should be logged using
+// log.Print.
+func (s *sasslang) GenerateRules(args language.GenerateArgs) language.GenerateResult {
+	files, err := ioutil.ReadDir(args.Dir)


You should be able to use args.Subdirs, args.RegularFiles, and args.GenFiles instead of reading the directory again.

Avoid panicking on I/O errors. Most errors in GenerateRules should simply be logged.

jayconrod · 2019-03-01T16:11:54Z

gazelle/sass.go

+	// base is the last part of the path for this element. For example:
+	// "hello_world" => "hello_world"
+	// "foo/bar" => "bar"
+	base := filepath.Base(args.Rel)


Rel will be slash-separated, so use path.Base instead of filepath.Base.

Note that Rel will be "" for the root directory, and path.Base will return "." for that, so you'll probably want a special case.

jayconrod · 2019-03-01T16:17:03Z

gazelle/sass.go

+			// Libraries in Sass have filenames that start with _.
+			base = filepath.Base(f)
+
+			rule := rule.NewRule("sass_library", base[1:len(base)-5])


So each _ file gets its own sass_library rules? You'll probably want a way to delete sass_library rules after those files have been removed. You can read rules in args.File and generate empty rules (returned through GenerateResult.Empty) if no rules are generated with those names. The empty rules will be merged, and if there's nothing left, the merged rules will be deleted.

Since I only expect there to be a single sass_library entry per directory by the name of the directory (if you're in //foo, the target would be //foo:foo), I've decided to add an empty rule.NewRule("sass_libary", base) to the Empty list in all cases. Since Gazelle does a resaonble thing and doesn't delete it if it was generated and deleted this seems like a safe decision. Can you please review this logic.

jayconrod · 2019-03-01T16:18:22Z

gazelle/sass.go

+func (s *sasslang) Kinds() map[string]rule.KindInfo {
+	return map[string]rule.KindInfo{
+		"sass_library": {
+			MatchAny: true,


MatchAny means that when merging a sass_library, the merger can match any target with the kind sass_library, regardless of name. This probably doesn't make sense if there may be multiple sass_library targets in the same build file.

jayconrod · 2019-03-01T16:20:37Z

gazelle/testdata/BUILD.bazel

@@ -0,0 +1,7 @@
+# gazelle:exclude .


An exclude directive can prevent Gazelle from recursing into a subdirectory, but a directory can't exclude itself. So this should be in the parent.

This has been resolved upstream, but I also fixed it in the PR.

achew22 · 2019-03-11T01:49:09Z

@nex3

I think this comment is still unaddressed...

To quote the actual comment (which I apparently can't reply directly to in Github?!?!)

I don't understand why this scans numbers, functions, hashes, URLs, or any sigils other than @ and ,. All you care about is finding the string @import outside of a comment, and reading strings after it.

Sorry for not responding to this earlier, I find the GitHub review interface a great reminder of how wonderful Critique + Gerrit are.

It scans numbers for a very simple reason, the CSS3 spec specifies how that they should be handled and it is known correct. When implementing a partial parser for SASS files, I was able to come up with examples where something fell through the cracks and didn't lex well. To correct for these, I just implemented that section of the CSS3 spec and any function that was called from inside of that function according to the spec. By the time I finished getting the parser to get all the testcases I could think of, I was almost all the way to a full parser, probably close to 90% and adding in the rest wasn't particularly difficult or very many lines of code.

nex3 · 2019-03-11T21:47:55Z

Sorry for not responding to this earlier, I find the GitHub review interface a great reminder of how wonderful Critique + Gerrit are.

I agree strongly 😝.

It scans numbers for a very simple reason, the CSS3 spec specifies how that they should be handled and it is known correct. When implementing a partial parser for SASS files, I was able to come up with examples where something fell through the cracks and didn't lex well. To correct for these, I just implemented that section of the CSS3 spec and any function that was called from inside of that function according to the spec. By the time I finished getting the parser to get all the testcases I could think of, I was almost all the way to a full parser, probably close to 90% and adding in the rest wasn't particularly difficult or very many lines of code.

What cases are you thinking of? Numbers can never contain the character @, nor can they contain quotes or /, so they shouldn't ever be a problem for lexing when looking for @import. Same goes for the other tokens that I'm advocating you ignore.

I really think the best approach here (in terms of producing a small amount of code with high accuracy) is to just blindly consume characters until you hit /*, ", ', \, or @import. But if you're willing to maintain the extra code that comes from lexing every CSS production, I suppose that's all right.

alexeagle · 2019-07-18T23:50:17Z

Hey there, what does this need to proceed? There's a team in Bazel that owns BUILD file generation now. Maybe they could poke at this?

achew22 · 2019-07-19T05:34:29Z

@alexeagle, I don't think there is anything holding this back other than Windows compatibility, for which I don't have a machine. I don't know exactly what's necessary here but I don't imagine it is much.

Once this is merged, would they be willing to work in it out of google3 or third party?

nex3 · 2019-08-09T22:37:09Z

@alexeagle If that team would be willing to take this on, that would be awesome. Do you know if they mostly use Gazelle for their tooling, or something else? Looking through the Gazelle README, it seems to be saying that it's built just for Go projects, which confuses me.

This change uses Gazelle (https://github.com/bazelbuild/bazel-gazelle), a BUILD file generator for Bazel projects, to generate rules_sass BUILD files automatically based on the directory structure of a preexisting project. Gazelle recently added support for plugins and I wanted to try writing a new plugin for it. Gazelle intelligently manages your BUILD files. It doesn't delete any rules (unless it is appropriate to delete it), operates off of a cache, and is fast enough to be used by the Kubernetes project for managing their BUILD files. Why should this be in the rules_sass repo? Great question! I believe this should live in the rules_sass repo so that the generator of BUILD files and the macros/rules in the repo can be iterated on in lock step. This will help prevent circumstances where a user of rules_sass forgets to update their dependency on Gazelle and ends up generating garbage BUILD files that are only logically to a former state of the rules_sass project. How to use this: 1. Follow the instructions on the Gazelle website for configuring Gazelle. 2. In the `BUILD` file in the root of your directory, amend the list of installed languages to additionally include the target, which is created in this commit, "@io_bazel_rules_sass//gazelle:go_default_library", 3. `cd` into the directory you would like to generate BUILD files for 4. `bazel run //:gazelle` 5. Enjoy your new BUILD files.

alexeagle · 2019-08-13T22:20:24Z

It's Patrick Doyle and team (stopwatch team) who own build_cleaner, mostly concerned with Java. They aren't doing Bazel/external yet but it's on their roadmap to think about it. Gazelle seems like a likely mechanism since it already has some adoption.
Gazelle was built for Go and the multi-language support was added later. I imagine the documentation is lagging, or maybe there just isn't much happening with other languages yet.

nex3 · 2019-08-14T21:28:13Z

Great, thanks! I've reached out to get his take on what the future of BUILD tooling plugins will look like.

ashi009 · 2019-11-29T02:06:30Z

9 months later, this is still not merged. :(

mrmeku · 2019-12-05T17:04:29Z

@nex3 @alexeagle Can you two clarify what work is left to be done to get this merged? I have a windows machine and and a willingness to subject myself to windows development for a good cause

nex3 · 2019-12-17T23:22:59Z

I'd like to see a dramatically simpler parser that only covers the necessary tokens, and all the tests need to pass.

vpanta · 2020-07-15T18:00:11Z

gazelle/sass/sass.go

+
+			rules = append(rules, rule)
+		} else {
+			normalFiles = append(normalFiles, f)


Won't this make the imports array and the rules array not have the same length (or be associated by index?)
GenerateResult implies they need exactly the same number of items as Resolve will be called with each index in tandem?

(I'm using your PR to get a feel for how gazelle generation works, so I'm new to this and might not be following correctly)

If you're looking for a better example, you can check out bazelbuild/bazel-skylib#251

Ok, seeing the setPrivateAttr(config.GazelleImportsKey) part makes me realize I have no idea why GenerateResult has Gen and Imports...

Sorry for the spam, like I said just wrapping my head around this.

If you're looking for a better example, you can check out bazelbuild/bazel-skylib#251

ty, I'll take any example I can get 😄

achew22 requested review from davidzchen, jelbourn and nex3 as code owners February 18, 2019 21:25

googlebot added the cla: yes label Feb 18, 2019

jelbourn requested a review from alexeagle February 19, 2019 21:54

jelbourn reviewed Feb 19, 2019

View reviewed changes

nex3 requested changes Feb 19, 2019

View reviewed changes

achew22 force-pushed the tokenizer branch 2 times, most recently from dd4d8b1 to cffc2c2 Compare February 24, 2019 23:41

jayconrod reviewed Mar 1, 2019

View reviewed changes

jayconrod mentioned this pull request Apr 16, 2019

[Feature Request]: Support a "fix deps" mode bazelbuild/bazel-gazelle#496

Closed

achew22 mentioned this pull request Jun 13, 2019

FYI - Gazelle for JS bazelbuild/bazel-gazelle#546

Closed

robfig mentioned this pull request Jun 13, 2019

Create a rules_closure BUILD file generator bazelbuild/rules_closure#394

Open

achew22 added 5 commits August 9, 2019 17:31

Respond to reviewer comments

0d1797d

Fix windows tests

f7c6709

Remote unused unquoteSassString function

99baa23

Respond to reviewer comments

26e65cf

achew22 force-pushed the tokenizer branch from 88669fd to 26e65cf Compare August 9, 2019 23:32

Sync with master

4c6f9f5

nex3 mentioned this pull request Sep 25, 2019

multi_sass_binary permission denied exclusive packages same directory #97

Open

alexeagle mentioned this pull request Sep 26, 2019

Create a rules_sass BUILD file generator #66

Closed

vpanta reviewed Jul 15, 2020

View reviewed changes

achew22 closed this Jul 15, 2020


		got := sassFileInfo(dir, tc.name)

		// Reexpose the fields we care bout for testing.

Create a rules_sass BUILD file generator #75

Create a rules_sass BUILD file generator #75

Conversation

achew22 commented Feb 18, 2019

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 left a comment

Choose a reason for hiding this comment

achew22 commented Feb 24, 2019

jelbourn commented Feb 25, 2019

nex3 commented Feb 26, 2019

jayconrod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achew22 commented Mar 11, 2019

nex3 commented Mar 11, 2019

alexeagle commented Jul 18, 2019

achew22 commented Jul 19, 2019

nex3 commented Aug 9, 2019

alexeagle commented Aug 13, 2019

nex3 commented Aug 14, 2019

ashi009 commented Nov 29, 2019

mrmeku commented Dec 5, 2019

nex3 commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment