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

toolkit: Allow an absolute path for app-device-config.json. #8

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

dpgeorge
Copy link
Member

This PR does two things:

  1. simplifies the use of paths.CONFIG_INPUT_DIR so it's only used in one location, which allows removing this global config variable altogether
  2. allows the app-device-config.json file to be relative (relative to --config-dir as before), or absolute

@dpgeorge dpgeorge requested a review from iabdalkader March 17, 2025 12:32
@iabdalkader
Copy link
Collaborator

If it's only used in location, may as well just make it --config-file? No?

@dpgeorge
Copy link
Member Author

If it's only used in location, may as well just make it --config-file? No?

Well... then there's no point in having it in the other json file... yes there are too many json config files!

@iabdalkader
Copy link
Collaborator

Well... then there's no point in having it in the other json file... yes there are too many json config files!

I agree. How about this: if the "binary" is not in "DEVICE" section, use --config-file?

@dpgeorge
Copy link
Member Author

How about remove --config-dir altogether, don't add --config-file, and just require an absolute path? That's very simple and covers all cases.

@dpgeorge
Copy link
Member Author

How about remove --config-dir altogether, don't add --config-file, and just require an absolute path?

Actually, no, that's not very friendly when you have hard-coded config files. It's only good for generated config files (where you have the luxury to write out the abs path).

Then maybe we go with --config-file and if it's specified then it overrides the value in the json config? (Options usually override default values.)

@iabdalkader
Copy link
Collaborator

Then maybe we go with --config-file and if it's specified then it overrides the value in the json config? (Options usually override default values.)

Yes, best way I think. Also specifying a relative/filename, in json file, and using the default config dir should still work then.

@iabdalkader
Copy link
Collaborator

Then maybe we go with --config-file and if it's specified then it overrides the value in the json config? (Options usually override default values.)

I'd also make "binary" optional, this way we don't have to provide any dummy value.

@dpgeorge
Copy link
Member Author

Yes, best way I think. Also specifying a relative/filename, in json file, and using the default config dir should still work then.

OK, so the logic is then:

  • if --config-file is specified then that overrides everything
  • else uses the file from the config json and prepends the toolkit build/config dir

But, that means you can really only use the second option there if you want one of the configs provided by this toolkit. If you want something else you must use the --config-file option. I guess that's fine.

@iabdalkader
Copy link
Collaborator

But, that means you can really only use the second option there if you want one of the configs provided by this toolkit. If you want something else you must use the --config-file option. I guess that's fine.

It is fine for our use case, if someone else is going to use the tools we could just as easily support both? For example:

if  args.config_file is not None:
   input_device_config = args.config_file
elif "binary" in sec:
   input_device_config = sec["binary"]
else:
   raise Exception("...")

if not os.path.isabs(input_device_config):
    input_device_config = os.path.join(args.config_dir, input_device_config)

@dpgeorge dpgeorge force-pushed the allow-absolute-device-config branch 2 times, most recently from b845c37 to d38fdf2 Compare March 17, 2025 13:04
@dpgeorge
Copy link
Member Author

I've allowed the value in the config file to be relative (to toolkit) or absolute.

The --config-file option can also be relative or absolute, but if it's relative then it's left alone (and is assumed relative to where you run the app-gen-toc.py from).

@iabdalkader
Copy link
Collaborator

iabdalkader commented Mar 17, 2025

So we can specify one of the json files in the toolkit, right? binary=app-device-config-ae3.json would work, right?
So in mpconfigboard.mk I could just remove the board-specific json and use:

ALIF_TOOLKIT_CFG_FILE = \"app-device-config-ae3.json\"

Also, should the option be --cfg- similar to the other ones?

@dpgeorge
Copy link
Member Author

So we can specify one of the json files in the toolkit, right? binary=app-device-config-ae3.json would work, right?

Yes, it should work (although I didn't test it yet).

Also, should the option be --cfg- similar to the other ones?

Maybe? Those other ones are for a different cfg json file though. But, yeah, could call it --cfg-binary to indicate that it overrides the "binary" field (even though it's not a binary...).

@iabdalkader
Copy link
Collaborator

Maybe? Those other ones are for a different cfg json file though. But, yeah, could call it --cfg-binary to indicate that it overrides the "binary" field (even though it's not a binary...).

They are, but form a user's point of view, they're all just config options, so probably easier to use the same for all (cfg or config). I noticed the binary name too, I guess it's because it gets compiled into a binary eventually 🤷🏼‍♂️

And remove --config-dir, it's no longer needed.

Signed-off-by: Damien George <[email protected]>
@dpgeorge dpgeorge force-pushed the allow-absolute-device-config branch from d38fdf2 to b15e218 Compare March 17, 2025 13:33
@dpgeorge
Copy link
Member Author

Updated to use --cfg-binary.

@iabdalkader
Copy link
Collaborator

I've tested this, with my CPP PR, and it seems to be working fine. I only tested specifying config in json's binary.

@iabdalkader
Copy link
Collaborator

iabdalkader commented Mar 17, 2025

@josuah FYI, we're removing --config-dir. You'll be able to specify a path to json config file with --cfg-binary or specify one in the AToC json file, which can be absolute or relative (for example one of the defaults in toolkit/build/config).

@dpgeorge
Copy link
Member Author

Tested all configurations:

  • --cfg-binary specified to override json, as both an absolute and relative path (relative to where the script was run from)
  • relative and absolute paths specified in the json

They all work.

@dpgeorge dpgeorge merged commit b15e218 into work-v1.104.0 Mar 18, 2025
2 checks passed
@dpgeorge dpgeorge deleted the allow-absolute-device-config branch March 18, 2025 00:58
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