-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
feat(idf.py): Allow adding arguments from file via @filename.txt (#11783) (IDFGH-10584) #11821
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @nebkat for the contribution. Could you please add a test for this in https://github.com/espressif/esp-idf/blob/master/tools/test_idf_py/test_idf_py.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebkat Thank you for bringing this useful feature to idf.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebkat Thank you for your improvement. I left a couple of notes to improve
Will make these changes now + will introduce possibility of recursive @ parameters (with check for circular dependency of course). Re the various changes suggested - this was copied directly from |
Now supports recursive expansion (e.g. @profile/device_v2_release contains @Release @device_v2 @partition_16mb). Tests written but I couldn't get them to run here, let me know if any changes needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nebkat Thank you for addressing the comments. I left few more with the solution how to fix the failing test and pipeline. Please address also those, then we can merge your PR.
Btw very nice work! We appreciate it.
tools/test_idf_py/args_circular_a
Outdated
@@ -0,0 +1 @@ | |||
-DAAA @args_circular.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-DAAA @args_circular.txt | |
-DAAA @args_circular_b |
tools/idf.py
Outdated
for line in f: | ||
expanded_args.extend(expand_args(shlex.split(line), os.path.dirname(rel_path), file_stack + [file_name])) | ||
except IOError: | ||
file_stack_str = " -> ".join(["@" + f for f in file_stack + [file_name]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase to the current master branch. The fix for the failing pipeline is present there.
The pre-commit will maybe raise some errors that should be fixed. One of them will be to change the double quotations "
for single quotations '
in ifd.py where possible.
file_stack_str = " -> ".join(["@" + f for f in file_stack + [file_name]]) | |
file_stack_str = ' -> '.join(['@' + f for f in file_stack + [file_name]]) |
expanded_args.extend(expand_args(shlex.split(line), os.path.dirname(rel_path), file_stack + [file_name])) | ||
except IOError: | ||
file_stack_str = " -> ".join(["@" + f for f in file_stack + [file_name]]) | ||
raise FatalError(f"File '{rel_path}' (expansion of {file_stack_str}) could not be opened. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the "
is okay, since '
are used in the string.
sha=4c1f574c3d495d292568a91dd4990d796f1cf3ee |
@nebkat Thanks for the contribution again. We will take care of it from now. No need to fix the pipeline. |
- moved test inputs to one directory - removed `-` from test arguments Closes espressif#11821 Closes espressif#11783
- moved test inputs to one directory - removed `-` from test arguments Closes espressif#11821 Closes espressif#11783
- moved test inputs to one directory - removed `-` from test arguments Closes espressif#11821 Closes espressif#11783
- moved test inputs to one directory - removed `-` from test arguments Closes espressif#11821 Closes espressif#11783
See discussion in #11783.
Replicates functionality from
esptool.py
allowing arguments to be loaded from a file, simplifying multiple build profiles.See espressif/esptool@d9b82fa