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

feat(idf.py): Allow adding arguments from file via @filename.txt (#11783) (IDFGH-10584) #11821

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

nebkat
Copy link
Contributor

@nebkat nebkat commented Jul 6, 2023

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

@espressif-bot espressif-bot added the Status: Opened Issue is new label Jul 6, 2023
@github-actions github-actions bot changed the title feat(idf.py): Allow adding arguments from file via @filename.txt (#11783) feat(idf.py): Allow adding arguments from file via @filename.txt (#11783) (IDFGH-10584) Jul 6, 2023
Copy link
Collaborator

@dobairoland dobairoland left a 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?

tools/idf.py Show resolved Hide resolved
tools/idf.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@kumekay kumekay left a 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

tools/idf.py Show resolved Hide resolved
tools/idf.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mfialaf mfialaf left a 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

tools/idf.py Outdated Show resolved Hide resolved
tools/idf.py Outdated Show resolved Hide resolved
tools/idf.py Outdated Show resolved Hide resolved
@nebkat
Copy link
Contributor Author

nebkat commented Jul 12, 2023

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 esptool.py, so some of these should possibly be applied there too:

https://github.com/espressif/esptool/blob/091c4a4149943828a7d8ace01ea3e6a448a8da2f/esptool/__init__.py#L926-L938

@nebkat
Copy link
Contributor Author

nebkat commented Jul 13, 2023

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.

Copy link
Collaborator

@mfialaf mfialaf left a 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.

@@ -0,0 +1 @@
-DAAA @args_circular.txt
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-DAAA @args_circular.txt
-DAAA @args_circular_b

tools/test_idf_py/args_circular_b Outdated Show resolved Hide resolved
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]])
Copy link
Collaborator

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.

Suggested change
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. "
Copy link
Collaborator

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.

@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Jul 17, 2023
@dobairoland
Copy link
Collaborator

sha=4c1f574c3d495d292568a91dd4990d796f1cf3ee

@dobairoland dobairoland added the PR-Sync-Merge Pull request sync as merge commit label Jul 21, 2023
@mfialaf
Copy link
Collaborator

mfialaf commented Jul 21, 2023

@nebkat Thanks for the contribution again. We will take care of it from now. No need to fix the pipeline.

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Status: Reviewing Issue is being reviewed Resolution: NA Issue resolution is unavailable labels Jul 21, 2023
@espressif-bot espressif-bot merged commit 4c1f574 into espressif:master Aug 1, 2023
6 of 7 checks passed
nebkat pushed a commit to nebkat/esp-idf that referenced this pull request Aug 28, 2023
- moved test inputs to one directory
- removed `-` from test arguments

Closes espressif#11821
Closes espressif#11783
nebkat pushed a commit to nebkat/esp-idf that referenced this pull request Nov 17, 2023
- moved test inputs to one directory
- removed `-` from test arguments

Closes espressif#11821
Closes espressif#11783
nebkat pushed a commit to nebkat/esp-idf that referenced this pull request May 8, 2024
- moved test inputs to one directory
- removed `-` from test arguments

Closes espressif#11821
Closes espressif#11783
nebkat pushed a commit to nebkat/esp-idf that referenced this pull request May 15, 2024
- moved test inputs to one directory
- removed `-` from test arguments

Closes espressif#11821
Closes espressif#11783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants