Use `require` wherever possible #27

tomodachi94 · 2022-12-09T03:16:30Z

This PR is in a highly unfinished state. I don't recommend merging it until it's ready.

Checklist:

Move files to .lua extension
Have APIs (technically will be modules after this) return all of their functions inside of a table
Add a shim for os.loadAPI that transparently requires modules when os.loadAPI() is called
Figure out how to get require added on every boot
Insert the necessary code to allow require to work: package.path = package.path .. ";/modules/?;/modules/?.lua;/modules/?/init.lua"
Test everything and fix all the bugs

Closes #21

Modules:

tomodachi94 · 2022-12-09T03:18:13Z

cc @danports

tomodachi94 · 2022-12-09T03:28:38Z

@danports should we rename util to prismutil or something? I feel like it is super ambiguous.

''You're a module now, Jimmy!''

danports · 2022-12-09T03:37:02Z

I agree that util isn't a great name. Looking at the functions in that package, I'd probably favor a name related to tables (tablehelper?) - admittedly still pretty generic, but then most of the API names in prism-core are like that. I'd suggest tackling the rename in a follow up PR though so the scope of this PR doesn't grow too large.

tomodachi94 · 2022-12-09T03:38:44Z

Also, what are your thoughts about automatic linting? I can also setup some CI to do linting in a separate PR.

danports · 2022-12-09T03:45:19Z

Re: linting, my feeling is that it depends on how strict it is. Certainly a CI pipeline that checks for syntax errors at a bare minimum would be better than what we have now (nothing).

tomodachi94 · 2022-12-09T03:47:24Z

Re: linting, my feeling is that it depends on how strict it is. Certainly a CI pipeline that checks for syntax errors at a bare minimum would be better than what we have now (nothing).

I've got a working CI setup that uses Selene, a configurable linter. In its default configuration, it checks for all sorts of things, and can check even more if you configure it to do so. It also has support for knowing about custom globals (ex. ComputerCraft APIs). To top it all off, it's written in Rust.

danports · 2022-12-09T03:50:45Z

Sounds good. I have pushed broken code to git before, though fortunately I've caught it by other means (e.g. canary deployments) before I had to fix hundreds of broken computers! 😅 Anything to prevent that would be helpful.

Also trimmed some ending whitespace.

Also trim some trailing whitespace.

tomodachi94 · 2022-12-09T04:51:02Z

We can squash the Convert... commits into one when we're finished, I'm doing atomic commits in case I make a mistake.

Also removed trailing whitespace and deleted the `require` for an unused library.

By default, package.path doesn't search for /modules. This changes that.

tomodachi94 · 2022-12-09T15:01:11Z

Alright, all of the APIs have been converted to modules.

tomodachi94 · 2022-12-09T15:08:20Z

@danports for the require shim, I think we'll need to introduce a script that runs everything that is present inside of the startup.d folder, but as such, we'd also need to modify railmanager in the prism-rails repository to use startup.d.

tomodachi94 · 2022-12-09T19:20:55Z

I've just written a script that does the heavy lifting of loading startup.d. It's available here. I've added it to this PR so we can automatically add cc-require on old versions. It will also help with the os.loadAPI shim.

(We'd need to migrate a single script in prism-rails to use this, but otherwise it wouldn't break anything.)

tomodachi94 · 2022-12-10T00:53:52Z

Only thing that appears to be left is the os.loadAPI shim. (Maybe we should do some testing too.)

This uses https://github.com/tomodachi94/cc-require, a fork of https://github.com/oddstr13/cc-require. Startupmgr is a script that executes everything in '/startup.d', and is useful for executing multiple scripts on boot.

danports · 2022-12-10T02:05:06Z

I am wondering whether it would be better to handle the require polyfill at the package/API level, or at the ROM/BIOS level. I see you did something similar for pastebin. Could we use a similar approach here? The other option would be to do something like os.loadAPI("require") or dofile("cc-require") at the top of every startup file, which seems feasible but a little inelegant. What do you think?

Also, yes, I'll do some testing on my server on this branch once it's ready.

danports · 2022-12-10T02:10:34Z

OK, I could see how you could do this without adding a line to the top of the files by moving all of the existing startup files into startup.d for all packages with such an entry point, which is probably better regardless, but I'm still wondering whether this should be in the library or the ROM. Given that require is present in the ROM in later CC versions/forks, I tend to think the polyfill should be at the ROM level too.

tomodachi94 · 2022-12-10T02:34:18Z

...I'm still wondering whether this should be in the library or the ROM. Given that require is present in the ROM in later CC versions/forks, I tend to think the polyfill should be at the ROM level too.

I agree that it should, and cc-require is intended to be used as a resource pack that modifies ROM (but it works in non-ROM too).

My modification to cc-require detects that if require as a function already exists, it will abort. In other words, if we have the patch on the builtin side, ROM side and on the non-ROM side, the builtin side will take precedence, followed by ROM, followed by local.

To summarize, I think it should remain in the library but a ROM patch should be available.

danports · 2022-12-10T02:44:56Z

Yeah, having both library and ROM options makes a lot of sense. So the idea is that requiem is an optional package that can be installed by the end user on computers that would need it? We wouldn't need to take requiem as a dependency for every package that uses require, right?

tomodachi94 · 2022-12-10T02:47:12Z

Yeah, having both library and ROM options makes a lot of sense. So the idea is that requiem is an optional package that can be installed by the end user on computers that would need it? We wouldn't need to take requiem as a dependency for every package that uses require, right?

Yep, pretty much. Originally, I was going to add the os.loadAPI shim to requiem, but having it in a separate package makes a lot more sense now.

This is needed to retain compatibility with unpublished scripts that use os.loadAPI().

tomodachi94 · 2022-12-10T03:08:01Z

@danports I've finished the os.loadAPI shim, this PR is ready for testing. I'm going to make an equivalent PR on prism-rails that should get merged after this one.

src/loadshim/startup.d/01-loadapi.lua

danports · 2022-12-10T03:42:09Z

Thanks! I'll take a look this weekend and see if I can test both this and the prism-rails PR. Some code in amber will likely need updating as well.

danports · 2022-12-11T20:36:56Z

Looking at this now...I think I need to figure out a solution for the amber client before I can go much further since I'll be able to pull the updated prism-core packages with the old amber but then if the amber client isn't compatible with those new packages, I'll have a broken installation. The autostartup solution is fine for daemon-type apps, but I'm not sure that's the right approach conceptually for command line apps like amber (well, amber is also a library, but let's leave that aside for now), since the startup.d directory mixes blocking (e.g. message loops) and non-blocking code. It seems like the options are:

Add require-specific code to amber (and any other command line apps that don't use autostartup).
Separate blocking and non-blocking code in autostartup.

What are your thoughts? I'm leaning towards 1️⃣ because I'm not sure it really makes sense to take a dependency on autostartup in command line apps when all we really want is require support (i.e. just one of the potential startup scripts). Maybe what we really want is something like a polyfills.d that gets loaded for command line apps and before startup.d for daemon apps. 🤔

danports · 2022-12-11T20:41:17Z

src/autostartup/modules/autostartup.lua

@@ -0,0 +1,3 @@
+package.path = package.path .. ";/modules/?;/modules/?.lua;/modules/?/init.lua"
+local dns = require("dns")
+


It seems like the rest of this file's contents are missing? Ditto for every other package converted to a module - it seems like only the new modules have any actual code.

-.- Not sure what happened here. I'll try to fix this tomorrow.

I'll work on fix later today.

danports · 2022-12-11T20:42:38Z

src/startupmgr/.manifest

@@ -0,0 +1,3 @@
+{
+	description = "Execute all scripts in the /startup.d folder"


I'm wondering whether we should merge startupmgr with autostartup. Thoughts?

Maybe. What are their differences?

tomodachi94 · 2022-12-12T17:58:04Z

In regards to Amber, I think we should do #1.

Add .lua extensions to Lua files

2f924f9

tomodachi94 added 2 commits December 8, 2022 19:34

Move APIs to module dir

e76c99f

''You're a module now, Jimmy!''

Convert autostartup to module

299bb1b

tomodachi94 force-pushed the use-require branch from 94ec800 to 75efbe8 Compare December 9, 2022 03:34

Convert dns to module

f5c978c

tomodachi94 force-pushed the use-require branch from 75efbe8 to f5c978c Compare December 9, 2022 03:35

tomodachi94 added 5 commits December 8, 2022 19:50

Convert events to module

8c75a71

Convert graphmanager to module

7ad1cd5

Also trimmed some ending whitespace.

Convert input to module

0b0fa60

Convert log to modules

7f467db

Convert messagebus to module

274274d

Also trim some trailing whitespace.

tomodachi94 added 6 commits December 8, 2022 21:23

Fix input

fb56f59

Convert net to module

8098d2c

Convert queuemanager to module

dfd1967

Also removed trailing whitespace and deleted the `require` for an unused library.

Convert util to module

3c01616

Convert wire to module

366843e

Prepend package.path to all modules

487ab8c

By default, package.path doesn't search for /modules. This changes that.

Bootstrap require() onto legacy systems

a8b2920

This uses https://github.com/tomodachi94/cc-require, a fork of https://github.com/oddstr13/cc-require. Startupmgr is a script that executes everything in '/startup.d', and is useful for executing multiple scripts on boot.

tomodachi94 force-pushed the use-require branch from 3a091b1 to a8b2920 Compare December 10, 2022 00:56

Add os.loadAPI shim

aaeeb3f

This is needed to retain compatibility with unpublished scripts that use os.loadAPI().

tomodachi94 mentioned this pull request Dec 10, 2022

Use require wherever possible danports/prism-rails#19

Open

danports reviewed Dec 10, 2022

View reviewed changes

src/loadshim/startup.d/01-loadapi.lua Outdated Show resolved Hide resolved

Fix syntax error

e7359f4

tomodachi94 marked this pull request as ready for review December 10, 2022 19:44

This was referenced Dec 10, 2022

Add automatic linting with GitHub Actions #28

Draft

Basic documentation #10

Open

danports reviewed Dec 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `require` wherever possible #27

Use `require` wherever possible #27

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 10, 2022

danports commented Dec 10, 2022

danports commented Dec 10, 2022

tomodachi94 commented Dec 10, 2022

danports commented Dec 10, 2022

tomodachi94 commented Dec 10, 2022 •

edited

Loading

tomodachi94 commented Dec 10, 2022 •

edited

Loading

danports commented Dec 10, 2022

danports commented Dec 11, 2022

danports Dec 11, 2022

tomodachi94 Dec 11, 2022 •

edited

Loading

tomodachi94 Dec 12, 2022

danports Dec 11, 2022

tomodachi94 Dec 12, 2022

tomodachi94 commented Dec 12, 2022

		@@ -0,0 +1,3 @@
		package.path = package.path .. ";/modules/?;/modules/?.lua;/modules/?/init.lua"
		local dns = require("dns")

		@@ -0,0 +1,3 @@
		{
		description = "Execute all scripts in the /startup.d folder"

Use require wherever possible #27

Are you sure you want to change the base?

Use require wherever possible #27

Conversation

tomodachi94 commented Dec 9, 2022 • edited Loading

Modules:

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

danports commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022

tomodachi94 commented Dec 9, 2022 • edited Loading

tomodachi94 commented Dec 9, 2022 • edited Loading

tomodachi94 commented Dec 10, 2022

danports commented Dec 10, 2022

danports commented Dec 10, 2022

tomodachi94 commented Dec 10, 2022

danports commented Dec 10, 2022

tomodachi94 commented Dec 10, 2022 • edited Loading

tomodachi94 commented Dec 10, 2022 • edited Loading

danports commented Dec 10, 2022

danports commented Dec 11, 2022

danports Dec 11, 2022

Choose a reason for hiding this comment

tomodachi94 Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

tomodachi94 Dec 12, 2022

Choose a reason for hiding this comment

danports Dec 11, 2022

Choose a reason for hiding this comment

tomodachi94 Dec 12, 2022

Choose a reason for hiding this comment

tomodachi94 commented Dec 12, 2022

Use `require` wherever possible #27

Use `require` wherever possible #27

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 9, 2022 •

edited

Loading

tomodachi94 commented Dec 10, 2022 •

edited

Loading

tomodachi94 commented Dec 10, 2022 •

edited

Loading

tomodachi94 Dec 11, 2022 •

edited

Loading