Codesearch: Add a general extractor for typescript files for validation purposes. [infra/infra : main]
Marc Jin (Gerrit)
Marc Jin added 1 comment![]( "Open in Gerrit")
func (m *newTsTarget) getDependencyFiles() []string {Junji WatanabeWouldn't it be easier to have GN handle this? Essentially this is reconstructing the dependency graph that GN already holds, right?
I'm not familiar with how the tsconfigs for webui are generated, but for devtools we chose to just emit the tsconfig_modified directly from gn, which is super simple.
Philip PfaffeWhen I was checking the "Ninja" build graph for devtools frontend's typescript targets, I noticed that they don't have complete dependencies for some reason. But if the build steps produce depfile. We can retrieve the dependencies from it.
Does devtools frontend's typescript targets produce depfiles?
Junji WatanabeNo we don't produce depfiles as far as I'm aware. How where the dependencies incomplete? I'm not aware of any dependency issues with the devtools build.
Note that that's also orthogonal to the problem here. The dependencies between tsconfig files is expressed via `references` entries. When I set up the indexer, the indexer wasn't able to deal with the references, which is why typescript.go ingests a singleton tsconfig that references all the files. The key difference between that and this CL is that here we're building the singleton tsconfigs manually in the indexer pipeline, whereas in devtools it just falls out of gn.
Marc JinHow where the dependencies incomplete? I'm not aware of any dependency issues with the devtools build.
About 2 years ago, I was trying to support remote execution for devtools typescript. http://b/308405411
We added a logic to scan the tsconfig file to find all deps because Ninja deps aren't sufficient.
https://source.chromium.org/chromium/chromium/src/+/main:build/config/siso/devtools_frontend.star;l=85;drc=bc0bd277c9a2fcde9989fbfe981344db0648f0b7I vaguely remember that this approach wasn't enough for some targets. But don't remember concrete examples of why it didn't work.
(Note that Ninja dependency graph isn't same with GN's graph. So, the deps aren't complete at least at Ninja deps layer. We often have to do a similar deps scanning from the command line for other build steps, too.)
Marc JinWouldn't it be easier to have GN handle this?
I'm fine with doing it in either places. I'm not familiar with GN and this seemed like an easier pick so I went along this route.
Do you know if this ts_library.py or the ts_config.json used somewhere else? If so, I think we want to preserve the original script/gn_targets/generated json file, but to add another script + gn_targets + generated file like tsconfig_modified.json. Does that mean we need to wrap the target and change every gn file like [1]? Does that mean in the gn_targets.json we need both wrapper target and the original target?
Btw [1] looks indeed much easier...
Marc JinI'm still taking a look at how everything is hooked in GN and I'm thinking about the scalability of this. For devtools you need a whole directory for the work https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/scripts/build/typescript/;bpv=1. Since the goal is to do it for all ts files in chromium, do you need different copies the scripts or just modifying the base ts_library.py would suffice?
Marc JinI just took a deeper look, IIUC, we should modify the ts_library() definiton to leverage siso to output the expanded dependencies into a argument, which ts_library.py has access to, and then update the ts_library.py to output those dependencies directly to the 'files' field in the json file.
Is it okay to just make changes to the current tools/typescript/ts_library{.py|.gni}? Would it affect other use cases?
Marc JinOh okay just feed in the dependent targets into generated_file() rule and read the output txt file will suffice?
Junji WatanabeJunji-san, you meant that using siso through generated_file() rule won't generate a whole list of dependencies for typescripts - maybe it's working for devtools but not for general TS files?
If that's the case would that leaves us only to adopt the solution as in this CL?
Marc JinIt looks like the `generated_file` may be able to collect transitive ts files.
Just to explain a bit more about this, `generated_file` allows you to collect "metadata" from the dependencies [1]. devtools `ts_library` template records the typescript files as metadata called "typescript_files" [2].Ideally, though, `siso query deps` should just work to provide dependencies for any targets. This is possible if ts_library step produces `depfile` [3]. Looking at the tsc flags, there is `--listFiles` option which is to `Print names of files part of the compilation` [4].
@pfa...@chromium.org
Does it make sense for `ts_library.py` to use `--listFiles` and generate a depfile?@jm...@google.com
The generated_file approach only collects `.ts` files, Does it need `.js` and other config files for codesearch indexing?[1] https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#func_generated_file
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/scripts/build/typescript/typescript.gni;l=253;drc=9fddfdafe79515f855072055229f1050b52d7124
[3] https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_depfile
[4] https://www.typescriptlang.org/tsconfig/#listFiles
Junji WatanabeI think the current approach Phillip did was to use generated_file() output all dependency files (direct and transitive), then that was consumed by ts_library() target as a "sourceslist" as gn target [1], which is eventually consumed by ts_target.py (script called by autoninja command) to write into tsconfig.json [2].
I think .ts and .d.ts is sufficient for our case!
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/BUILD.gn;l=192-206;bpv=1;bpt=0
[2] https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/devtools-frontend/src/frontend_indexer_tsconfig-tsconfig.json;l=37?q=rontend_indexer_tsconfig-tsconfig.json&ss=chromium
Philip PfaffeI found some issues that I faced with.
Looking at this Siso handler code that compliments missing deps, it tries to infer ".d.ts" location from the corresponding source or gen directories. (Just scanning tsconfig's "files" wasn't sufficient.)https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/build/config/siso/devtools_frontend.star;l=87-89?q=f:devtools%20f:star%20-file:%5Eout%20-file:%5Egen
(Note that this code isn't used in production since it was incomplete.)There are some discussions about missing ".d.ts" in https://crbug.com/40945932, https://crbug.com/40943433.
Philip Pfaffe--listFiles/depfile does not help (or did not, we should maybe double check if the indexer learned how to deal with `references` by now). DevTools actually has multiple compilation units. But the indexer isn't (wasn't) able to resolve the dependencies between them. --listFiles only produces the files for one compilation unit.
Demetrios Papadopoulos@jm...@google.com I would recommend trying the gn-based version of this logic for comparison. There's quite a bit of duplicated code no between here and the indexer_tsconfig handler. And it reconstructs quite a bit of information that already exists within gn.
If the current solution turns out to work better for webui, I'm not against it, but my intuition says that controlling this from gn is much simpler. Especially since there isn't really that much webui typescript, having a simpler solution is preferable in my eyes.
Marc JinEspecially since there isn't really that much webui typescript, having a simpler solution is preferable in my eyes.
Not sure how is considered much or not in this context, but TypeScript WebUI is the status quo across the WebUI codebase, already extensively used, and moreover the WebUI codebase is ever growing with more and more UIs in Chrome adopting WebUI.
dpapad@ and rebekah and I had a meeting and we agree moving to gn is a more decent solution. However without knowing if the current solution works with Kythe indexer I prefer conducting an experiment here, just for validation. Having such a script is a simple way to do that and also for debug? Agree we should move everything to gn later for greater benefit :) (for example siso can behnefit from it I believe)
Related details
- Demetrios Papadopoulos
- Junji Watanabe
- Nicole Desrosiers
- Philip Pfaffe
- Rebekah Potter
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicole Desrosiers (Gerrit)
Nicole Desrosiers added 5 comments![]( "Open in Gerrit")
type Reference struct {
Path string `json:"path"`
}
type CompilerOptions struct {
RootDir string `json:"rootDir"`
OutDir string `json:"outDir"`
Paths map[string][]string `json:"paths"`
}
type TsConfig struct {
Extends string `json:"extends"`
CompilerOptions CompilerOptions `json:"compilerOptions"`
Files []string `json:"files"`
References []Reference `json:"references"`
}Nit: blank line between each type?
tsconfig := target.Outputs[0]Nit: tsConfig, maybe?
files, ok := m.cache.GetFiles(unique.Make(m.tsconfig))
pathMap, okToo := m.cache.GetPathMapping(unique.Make(m.tsconfig))
if ok && okToo {
for _, v := range files {
dependencyFiles = append(dependencyFiles, v.Value())
}
for key, val := range pathMap {
for _, v := range val {
pathMapping[key.Value()] = append(pathMapping[key.Value()], v.Value())
}
}
return dependencyFiles, pathMapping
}Could you add a brief comment describing this codepath? It looks like we can shortcut here, but I'm not certain why.
if err == nil {Is it okay to ignore err in this case?
updatedJSON, err := json.MarshalIndent(config, "", " ")Nit: Is this correct casing?
Related details
- Demetrios Papadopoulos
- Junji Watanabe
- Marc Jin
- Philip Pfaffe
- Rebekah Potter
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Jin (Gerrit)
Marc Jin added 5 comments![]( "Open in Gerrit")
type Reference struct {
Path string `json:"path"`
}
type CompilerOptions struct {
RootDir string `json:"rootDir"`
OutDir string `json:"outDir"`
Paths map[string][]string `json:"paths"`
}
type TsConfig struct {
Extends string `json:"extends"`
CompilerOptions CompilerOptions `json:"compilerOptions"`
Files []string `json:"files"`
References []Reference `json:"references"`
}Nit: blank line between each type?
Done
tsconfig := target.Outputs[0]Marc JinNit: tsConfig, maybe?
Done
files, ok := m.cache.GetFiles(unique.Make(m.tsconfig))
pathMap, okToo := m.cache.GetPathMapping(unique.Make(m.tsconfig))
if ok && okToo {
for _, v := range files {
dependencyFiles = append(dependencyFiles, v.Value())
}
for key, val := range pathMap {
for _, v := range val {
pathMapping[key.Value()] = append(pathMapping[key.Value()], v.Value())
}
}
return dependencyFiles, pathMapping
}Could you add a brief comment describing this codepath? It looks like we can shortcut here, but I'm not certain why.
Yeah it's just querying the cache for two things, and if both found then we can early return.
Is it okay to ignore err in this case?
Yeah I'm thinking about it. We are sure that the address provided are absolute addresses so filepath.Abs won't even trying to look for pwd, but just compress the path (actually the main purpose is to compress the path I think).
e.g. what we fed in is sth. like '/usr/local/src/infra/../file', and we will just get '/usr/local/src/file'
so should be fine? wdyt?
updatedJSON, err := json.MarshalIndent(config, "", " ")Nit: Is this correct casing?
- Demetrios Papadopoulos
- Junji Watanabe
- Nicole Desrosiers
- Philip Pfaffe
- Rebekah Potter
Marc Jin (Gerrit)
Marc Jin added 1 comment![]( "Open in Gerrit")
Marc JinGreat work to get this working! This generally looks good to me, but see inline comment.
Done
Related details
- Demetrios Papadopoulos
- Junji Watanabe
- Nicole Desrosiers
- Philip Pfaffe
- Rebekah Potter
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |