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

mcrun and mcconfig should have the same env vars by default #1024

Closed
tve opened this issue Feb 3, 2023 · 5 comments
Closed

mcrun and mcconfig should have the same env vars by default #1024

tve opened this issue Feb 3, 2023 · 5 comments

Comments

@tve
Copy link
Contributor

tve commented Feb 3, 2023

By default, manifests processed by mcconfig can use $(MODULES) while manifests processed by mcrun cannot. Please make the default env vars the same (I didn't check $(PIU), etc). I realize this can be worked around by specifying an explicit build property but given that the docs for modules all have a section like

"include": [
		"$(MODULES)/data/base64/manifest.json"
	]

it's kind'a novice unfriendly.

Running mcrun without the work-around results in

### Error: 'undefined/data/base64/manifest.json': manifest not found!   
@tve tve changed the title mcrun should have the same env vars by default mcrun and mcconfig should have the same env vars by default Feb 3, 2023
@andycarle
Copy link
Member

I agree with your bottom line point here, but your statement of the problem is a little off in a way that matters when considering solutions.

By default, manifests processed by mcconfig can use $(MODULES) while manifests processed by mcrun cannot.

That is not quite right. What's actually happening is that most Moddable SDK examples (and, subsequently, most apps that people build) include "$(MODDABLE)/examples/manifest_base.json". manifest_base.json defines the variables that you want:

	"build": {
		"BUILD": "$(MODDABLE)/build",
		"MODULES": "$(MODDABLE)/modules",
		"COMMODETTO": "$(MODULES)/commodetto",
		"PIU": "$(MODULES)/piu"
	}

The right solution is probably to also include that block of variables in manifest_mod.json, which most Mods include. Could you try that out and see if it fixes this issue in a way that feels right to you? Just replace the contents of manifest_mod.json with:

{
	"build": {
		"BUILD": "$(MODDABLE)/build",
		"MODULES": "$(MODDABLE)/modules",
		"COMMODETTO": "$(MODULES)/commodetto",
		"PIU": "$(MODULES)/piu"
	},
	"platforms": {
        "esp32/m5atom_echo": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        },
        "esp32/m5atom_lite": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        },
        "esp32/m5atom_matrix": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        },
        "esp32/m5atom_u": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        },
		"esp32/m5stack_core2": {
			"build": {
				"UPLOAD_SPEED": "1500000",
				"DEBUGGER_SPEED": "921600"
			}
        },
        "esp32/m5stick_c": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        },
        "esp32/m5stick_cplus": {
            "build": {
                "UPLOAD_SPEED": "1500000",
                "DEBUGGER_SPEED": "1500000"
            }
        }
	}
}

@tve
Copy link
Contributor Author

tve commented Feb 4, 2023

Ah, thanks for the clarification, I had not looked where the variable definitions come from and had assumed they were in the tools. 🙄
I don't see why your proposed fix wouldn't work. I can't really test it on a real project 'cause ... well... pulling the base64 manifest into a mod ain't gonna work no matter what the env vars are 'cause it uses native code, ooops! 😬
NB: might be nice to add some "native code" tag in the docs to relevant modules...

@phoddie
Copy link
Collaborator

phoddie commented Feb 9, 2023

I don't think there's a strong reason to define PIU or COMMODETTO. The modules there aren't likely to be included by a mod, but provided by the host. I'm not sure BUILD is all that useful either. There's no harm in adding them, I suppose. But MODULE makes good sense

With tomorrow's Moddable SDK update, we'll reject attempts to use a native module in a mod at build time, so mistakes will be caught early.

@phoddie
Copy link
Collaborator

phoddie commented May 31, 2023

In fact, there is at least one Piu module that is JavaScript-only. So... in the interest of consistency, I've updated the build section of manifest_mod to match manifest_base.

@phoddie
Copy link
Collaborator

phoddie commented Jun 27, 2023

Closing as proposed change committed.

Note that there were also changes to provide much better error messages when attempting to use native modules with mcrun:

Native code detected:
  ~/moddable/modules/base/time/esp/modTime.c
  ~/moddable/modules/base/timer/modTimer.c
Mods cannot contain native code. Did you intend to build using mcconfig?
### Error: mod cannot contain native code

@phoddie phoddie closed this as completed Jun 27, 2023
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

No branches or pull requests

3 participants