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

New manifest.json property redirect #1425

Open
wants to merge 6 commits into
base: public
Choose a base branch
from

Conversation

cmidgley
Copy link
Contributor

When defining custom subplatforms, the manifest.json for esp32 sets the include directory for subplatforms to ./targets/$(SUBPLATFORM)/manifest.json. This assumes the target directory is in the same location as the manifest.json file. However, if you have many manifest.json projects in different directories, it seems there are two choices:

  1. Duplicate your target directory (or symlink it) for each project.
  2. Modify the Moddable platform manifest.json file(s), and merge on each Moddable release.

Unless there's a better/existing approach to solve this, this PR proposes a new uninclude property that can exclude manifests. For example, the following would allow a shared esp32 sub-platform directory for all manifest.json projects (affecting only the esp32 platforms):

	"platforms": {
		"esp32/*": {
			"uninclude": "./targets/$(SUBPLATFORM)/manifest.json",
			"include": "$(SRC)/build/targets/$(SUBPLATFORM)/manifest.json"
		}
	}

See manifest.md in the PR for documentation on this property.

NB: I'm not thrilled with the name uninclude but I didn't feel like generic names such as remove or exclude were appropriate, as they seemed easier to misunderstand.

@phoddie
Copy link
Collaborator

phoddie commented Oct 16, 2024

I understand the problem you are solving with this. I'm not sure about the solution. The uninclude/include are paired, but they may not appear that way in the manifests. Maybe redirect / replace is another way to think about?

"platforms": {
	"esp32/*": {
		"redirect": {
			"from": "./targets/$(SUBPLATFORM)/manifest.json",
			"to": "$(SRC)/build/targets/$(SUBPLATFORM)/manifest.json"
		}
	}
}

The redirect property could be an array of those objects as well.

That would work for manifests and sources. That could be useful for redirecting to alternate module implementations (like your patches WebSocket client).

Paths are usually evaluated relative to the manifest so "from" above would need to be $MODDABLE/build/device/esp32/targets/$(SUBPLATFORM)/manifest.json.

This is just a quick idea for discussion. It may well be flawed.

@cmidgley
Copy link
Contributor Author

Thanks for being supportive of a possible change.

Redirect is an interesting idea - it unifies the replacement and makes the association of what it is doing more obvious.

My original use of uninclude was to remove the rule in esp32 that breaks any build with a custom sub-platform name. Removing it allowed me to add platforms rules to various manifests with specific settings. Eventually, I did add sub-platform-specific manifests so redirect works for my current use case. But it feels like there are more use cases for removal without replacement, which led me to this idea...

I can see needing to adjust other manifest properties in the future, such as modules, resources, and preload. I've wanted for some time to be able to disable some/all preloads when debugging. This has me contemplate a more extensible structure such as this:

"platforms": {
	"esp32/*": {
		"transform": {
			"include": {
				"from": "./targets/$(SUBPLATFORM)/manifest.json",
				"to": "$(SRC)/build/targets/$(SUBPLATFORM)/manifest.json"
			},
			"preload": [
				{
					"remove": "some/preload/path"
				},
				{
					"remove-regex": "^mymodule/.*"
				}
			]
		}
	}
}

The biggest downside I see is how verbose it is: transform / include / from/to compared to uninclude or redirect/from/to. I would implement only the from/to functionality on `include, but we would have the structure in place for future needs.

Perhaps I'm over-thinking this. ;-)

@cmidgley
Copy link
Contributor Author

cmidgley commented Oct 17, 2024

Well, I just hit another issue. io has the $(SUBPLATFORM) problem, but this time with modules. As I dove into this, I realized the structure above won't work (modules is an object and not a simple string array).

Let me work on this a bit...

@phoddie
Copy link
Collaborator

phoddie commented Oct 17, 2024

In brief...
-I was imagining that the redirection could apply beyond include. Not sure about how to implement without making a mess of mcconfig/mcrun/mcmanifest though.

  • A redirect without a destination is effectively a remove

@cmidgley cmidgley changed the title New manifest.json property uninclude New manifest.json property redirect Oct 18, 2024
@cmidgley
Copy link
Contributor Author

cmidgley commented Oct 18, 2024

Take two. Per Peter's suggestion, it's now called redirect with from and to properties. It works at the root (across all) and on platform targets (under platforms), supports wildcards on from with null, and will delete by not including to (or setting it to null).

Work-in-progress: looking for feedback, but not ready to merge.

From the updated tools/manifest.md:

redirect

The redirect array can be used to redirect (or change) manifest properties that are defined in other manifest files. This is useful when including system manifests where properties need to be altered without editing the Moddable core files.

For example, to use a private directory for sub-modules, you can use an include redirection like this:

"redirect": {
	"include": {
		"from": "./targets/$(SUBPLATFORM)/manifest.json",
		"to": "$(SRC)/targets/$(PLATFORM)/$(SUBPLATFORM)/manifest.json"
	}
}

The properties include, strip, and preload (which are string arrays in the manifest) expect from and to for each property to redirect. The redirection rule can be a single object, or an array of objects:

"redirect": {
	"include": [
		{
			"from": "some-include-path",
			"to": "some-redirect-path"
		},
		{
			"from": "another-include-path",
			"to": "new-path"
		}
	]
}

The modules, resources, data, build, and config properties (which are objects in the manifest) expect a similar format, but with an object key qualifier:

"redirect": {
	"modules": {
		"embedded:provider/builtin": {
			"from": "./targets/$(SUBPLATFORM)/manifest.json",
			"to": "$(SRC)/targets/$(SUBPLATFORM)/manifest.json"
		}
	}
}

The value of null can be used as a wildcard on from to match all. For example, to disable all preloads and replace them with a specific list of modules:

"redirect": {
	"preload": {
		"from": null,
		"to": ["engine", "unit-test"]
	}
}

to can also use null to indicate the item should be deleted (if to is not provided, it is assumed to be a delete). For example, to remove all preloads:

"redirect": {
	"preload": {
		"from": null
	}
}

You can qualify redirect to apply only for specific platforms by placing it inside the platforms property. The following will replace the config.rotation value only for the esp32/m5stick_cplus platform:

"platforms": {
	"esp32/m5stick_cplus": {
		"redirect": {
			"config": {
				"rotation": {
					"from": "270",
					"to": "90"
				}
			}
		}
	}
}

@cmidgley
Copy link
Contributor Author

This is complete and ready for review/consideration. I've run it across all my projects (around 15) on win and esp32, as well as five example projects and it is working well.

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.

2 participants