-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Extract Go and Python Beam symbols for Playground #23378
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
Conversation
playground/frontend/playground_components/lib/src/services/symbols/loaders/yaml.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/loaders/yaml.dart
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/loaders/yaml.dart
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/loaders/yaml.dart
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/symbols_notifier.dart
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/symbols_notifier.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/symbols_notifier.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/symbols_notifier.dart
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/playground_components.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/lib/src/services/symbols/loaders/yaml.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/test/src/services/symbols/symbols_notifier_test.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/test/src/services/symbols/symbols_notifier_test.dart
Show resolved
Hide resolved
playground/frontend/playground_components/test/src/services/symbols/symbols_notifier_test.dart
Outdated
Show resolved
Hide resolved
playground/frontend/playground_components/test/src/services/symbols/loaders/yaml_test.dart
Show resolved
Hide resolved
|
Assigning reviewers. If you would like to opt out of this review, comment R: @Abacn for label build. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
playground/frontend/README.md
Outdated
| cd .. | ||
| flutter pub get | ||
| flutter pub run build_runner build | ||
| $ ./gradlew :playground:frontend:configure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the $? It makes it more convenient to copy/paste for users consuming the README instructions. There are many others below as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this command in two different environments and received two different errors. In the first environment, I cloned and checked out the branch in the Google Cloud shell. This led to several incremental retries with the message showing below.
Attempting `./gradlew :playground:frontend:configure` leads to `pub get failed (server unavailable) -- attempting retry 7 in 64 seconds...`
However, when I navigated to the playground/frontend directory and ran pub get, I didn't have any problems except for this message:
Git error. Command: `git clone --mirror https://github.com/BertrandBev/code_field.git /home/<user>/.pub-cache/git/cache/code_field-4fdb625753a3dad07e371ca34045120ddd9a3f88
stderr: Cloning into bare repository '/home/<user>/.pub-cache/git/cache/code_field-4fdb625753a3dad07e371ca34045120ddd9a3f88'...
So I attempted a second environment which was on my own linux machine.
Running the command in this context, yielded the following error.
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':playground:frontend:flutterClean'.
> A problem occurred starting process 'command 'flutter''
I ran the command again as ./gradlew :playground:frontend:configure --stacktrace and it yielded:
java.io.IOException: Cannot run program "flutter" (in directory "/playground/frontend"): error=2, No such file or directory
at net.rubygrapefruit.platform.internal.DefaultProcessLauncher.start(DefaultProcessLauncher.java:25)
... 6 more
Caused by: java.io.IOException: error=2, No such file or directory
However, running flutter --version, I see:
Flutter 3.3.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision e3c29ec00c (3 weeks ago) • 2022-09-14 08:46:55 -0500
Engine • revision a4ff2c53d8
Tools • Dart 2.18.1 • DevTools 2.15.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Cloud Shell, I narrowed down the problem and filed an issue:
https://issuetracker.google.com/issues/251039262
Can you please check if git clone --mirror also fails for you? If so, please upvote the issue and maybe provide additional details there (project ID?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the local machine, I cannot reproduce this, and I cannot really see how is Flutter in $PATH for the terminal but not in $PATH for Gradle ran from that same terminal. Can you show the output of these?
cat /etc/*-releasewhich flutterecho $PATH./gradlew :playground:frontend:printPath(a new task I just added)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the above commands will show I tested on Linux Debian and that flutter is in my path. I don't want to delay this PR further and thank you for this information. However, running ./gradlew :playground:frontend:configure resulted in a Module not found yaml error which I resolved by installing the missing python library. Now ./gradlew :playground:frontend:configure --stacktrace reveals this error:
> Task :playground:frontend:playground_components:extractBeamSymbolsPython FAILED
Traceback (most recent call last):
File "playground/frontend/playground_components/tools/extract_symbols_python/extract_symbols_python.py", line 109, in <module>
class_names = get_dir_symbols_recursive(args.dir)
File "playground/frontend/playground_components/tools/extract_symbols_python/extract_symbols_python.py", line 100, in get_dir_symbols_recursive
class_names.update(get_file_symbols(file_path))
File "playground/frontend/playground_components/tools/extract_symbols_python/extract_symbols_python.py", line 66, in get_file_symbols
[target] = member_node.targets
ValueError: too many values to unpack (expected 1)
| ### Docker | ||
|
|
||
| `$ flutter pub run build_runner build` | ||
| To run the frontend app with Docker, run this in the frontend directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Docker the only way to run this application? Is there any way that we can include instructions for developers who want to contribute, but who are prevented for various reasons from using Docker?
May we consider adding instructions for these developers, including where it is expected to execute flutter pub run build_runner build such as in the playground_components folder? I'm assuming such command runs in the context of the gradle command to setup the project, however, please see the errors I encountered when executing this setup command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docker is not required. The recommended way to run this locally is still flutter run.
I updated the README stating the cases when one might prefer Docker.
| from typing import List | ||
|
|
||
|
|
||
| def should_include_class_node(class_node: ast.ClassDef) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan for unit tests for this method as well as the other methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I made a test for the entire command in Dart.
First I thought about testing Python code in Python, but it has a drawback. We will have 3 more scripts for this job in other languages, very much different inside but with an identical API. In Dart we can make 4 nearly identical tests instead of 4 very different native tests on those languages. So the Dart solution is easier to maintain.
|
Reminder, please take a look at this pr: @Abacn |
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloem I wasn't able to fully review this PR but didn't want to hold it back.
Pros:
- flutter code is readable and
flutter runresults in a working frontend flutter testpasses
Opportunities:
- Not clear whether https://github.com/apache/beam/blob/master/playground/frontend/README.md?plain=1#L32-L37 will remain after this PR will merge and personally I think it helps to have these instructions
./gradlew :playground:frontend:configurekept throwing errors and I can't rule out whether it is my execution environment
| cd playground_components | ||
| flutter pub get | ||
| flutter pub run build_runner build | ||
| cd .. | ||
| flutter pub get | ||
| flutter pub run build_runner build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this PR still intend on deleting https://github.com/apache/beam/blob/master/playground/frontend/README.md?plain=1#L32-L37?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted these lines because with the introduction of symbols extraction the building got more complicated. That is why :playground:frontend:configure was introduced.
With the recent idea of keeping generated files under version control, :playground:frontend:configure may remain as simple as the above 6 commands. This means it can hardly break, so I guess it should be safe to delete them from the REAMDE. Otherwise we will have two locations to maintain the build commands:
:playground:frontend:configureGradle task.- README.
Yet the failure of Gradle on your machine with Flutter in path is a concern.
Do you want to keep those commands as a backup?
|
It's OK to hold this PR until everything builds on any machine. This does not block us because meanwhile we are busy on the editor. We expect to migrate to Akvelon editor next week. This means we will no longer have a git dependency which means Google Cloud Shell will start working. Meanwhile I will fix the Python script using your feedback and list |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @damccorm for label build. Available commands:
|
|
Reminder, please take a look at this pr: @damccorm |
|
stop reviewer notifications |
|
Stopping reviewer notifications for this pull request: requested by reviewer |
|
can you rebase this PR please @alexeyinkin ? |
# Conflicts: # .gitignore # playground/frontend/build.gradle # playground/frontend/playground_components/build.gradle.kts
|
@pabloem I merged master. |
|
oh I realize this was not lgtmd? @damondouglas should I review this? merge? waqit for your review? |
|
@damondouglas now that we use the editor from pub.dev instead of git, we do not hit the bug of Google Cloud Shell, and it works all the way from |
| - assets/translations/en.yaml | ||
|
|
||
| flutter_gen: | ||
| output: lib/src/generated/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**/generated/* is git-ignored on the repository level, so we evade that.
|
Meanwhile I also did
|
* Extract Beam symbols for Go SDK (apache#23917) * Fix after review, add tests (apache#23917) * Fix tests, lazy symbols loading on SDK switch (apache#23917)
# Conflicts: # learning/tour-of-beam/frontend/pubspec.lock # playground/frontend/playground_components/lib/src/controllers/playground_controller.dart # playground/frontend/playground_components/lib/src/controllers/snippet_editing_controller.dart
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexeyinkin Would the following remain an issue if we merged this PR?
1. I noticed that the text for some of the buttons/dropdowns were misaligned:
and
2. Possibly related to 1. There appears to be a size overflow error.
|
@damondouglas how did you build and run it to get that? My tests: 1. Cloud Shellcp playground/frontend/lib/config.example.dart playground/frontend/lib/config.g.dart
cd playground/frontend/
docker build -t playground-frontend .
docker run -p 8080:8080 playground-frontendFlutter 3.3.2, Chrome. 2. Local debug with Android Studiocp playground/frontend/lib/config.example.dart playground/frontend/lib/config.g.dartFlutter 3.3.7, Chrome. |
|
Good thank you. I executed the commands locally in the playground/frontend/README on a Linux Debian machine. I'm not remoting into the device currently to inform the flutter version but updated prior to execution. I'd say then this PR is good to go. |
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pabloem LGTM
|
lgtm thanks |





This PR:
The symbols are not yet used by the app after loading. The app will start using them when the Akvelon editor replaces the old editor. Still this PR is needed for easier testing of the Akvelon editor before the switch and to simplify. It also simplifies post-checkout tasks.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.