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

Build process improvements #122

Closed
wants to merge 11 commits into from
Closed

Conversation

ciscoheat
Copy link
Contributor

Improved the build process (mostly for VS Code), and streamlined the output when using -r.
-c and -r now also uses exit codes. (1 for compilation failed, 2 for "no input file found").

@ciscoheat
Copy link
Contributor Author

Now I've made so #123 works according to the idea I had. Now you can do (in a dir where you've unzipped the dist package):

bin/trygve -c examples/breakout.k -r

And it will start the breakout game in a window, by instantiating a hidden TextEditorGUI and starting the program from there.

It detects if a GUI is needed by a regexp, checking if a Frame is instantiated in the source code. A bit hacky, but it should work. I can't be bothered to parse the AST at the moment. :)

@jcoplien
Copy link
Owner

  1. I think your change thwarts much of the original motivation for -r.
  2. As you say, this is a bit hacky. A lot hacky. Just that a Frame exists in the code doesn't mean that the program will ever create it.
  3. We don't have a long line of people screaming for this. It increases code size and complexity with what is a best a marginal benefit, together with potential confusion.
  4. I don't understand what real, practical problem it solves.

I still think you perhaps don't understand the distinction I was trying to make with the -r command. It is in part a confirmation that the invoked program does not use graphics, so it is is safe to use in an unattended script. With your change, that is no longer true. I am strongly disinclined to approve this change.

Also, good git practice (and version management practice in general) separates individual features into individual deltas (or, as we tend to manage things here, into separate branches) that can individually be incorporated into the trunk. The VS build improvement (which also solves a problem that I didn't understand that we had, and which increases the tool inventory, and increases the product footprint) should have been on a separate branch, and certainly administered as a separate issue. Issue 122 is called "build process improvements" but there seems to be a lot more going on than that. Please break it out so we can administer the delta separately, as you did with 123 and 124 (but I need to look at them to ensure that they, too, are "pure.")

The existence of the tar file is important. Some environments cannot or at least should not download .zip files, as it is a security risk — unpacking the zip can have unwarranted side effects. Tarballs are considered safe. So this change also needs to be undone or rejected independently of the others. Again, these things should be discussed in the chat before making it all the way to a pull request.

There are changes on this branch I would strongly consider merging. At the very least, the other changes should be first discussed in the discussion group, as directed in the README. I think that the VS build improvements bear discussion. I also want to understand the antlr changes a bit more (and those also should be administered as an issue in their own right) before approving it — no one is complaining about the size of the distribution. When you say "distribution," do you mean the distribution of the .jar file or of the source?

I want to keep this environment as minimal as possible, and we should be careful to not be distracted by bright shiny new objects that end being distractions.

@ciscoheat
Copy link
Contributor Author

Of course, this must be split up into multiple PR:s, I've been experimenting with many things, so this is the culmination. I'll cherry pick the changes and make them separate, then post a link in the group about them.

@ciscoheat ciscoheat closed this Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants