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

Display full path to each processed file. #14

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

anakrish
Copy link
Contributor

@anakrish anakrish commented Jun 4, 2020

Displaying full path allows the user to figure out what file is being processed
in case an unintended file is being picked up from the specified search paths.
Add test for printed path.

Add tests to lock down the order in which search paths are processed.

Signed-off-by: Anand Krishnamoorthi [email protected]

Displaying full path allows the user to figure out what file is being processed
in case an unintended file is being picked up from the specified search paths.
Add test for printed path.

Add tests to lock down the order in which search paths are processed.

Fix utility function name typo.

Signed-off-by: Anand Krishnamoorthi <[email protected]>
@mingweishih
Copy link
Contributor

Shouldn't we make this as optional (i.e., a command-line option)?

@anakrish
Copy link
Contributor Author

anakrish commented Jun 5, 2020

Shouldn't we make this as optional (i.e., a command-line option)?

I am leaning towards not making it an option. oeedger8r always prints messages 'Generating code for Open Enclave SDK', 'Success' (in case of success). Printing the files being processed will add couple more lines to the output; but I don' think most users would care about these additional lines being printed in case of success. On the other hand, those that run into issues would find these lines useful.
What do you think?

@mingweishih
Copy link
Contributor

Shouldn't we make this as optional (i.e., a command-line option)?

I am leaning towards not making it an option. oeedger8r always prints messages 'Generating code for Open Enclave SDK', 'Success' (in case of success). Printing the files being processed will add couple more lines to the output; but I don' think most users would care about these additional lines being printed in case of success. On the other hand, those that run into issues would find these lines useful.
What do you think?

For the current status, I think it's fine. I'm just thinking for the long-term, if we consider this as a compiler-like tool, I think it makes sense to make this feature as a command-line option (and we may have many more). For example, a compiler-like tool typically provides -verbose option to display the detailed log. For more complicated ones like gcc/clang, there are multiple options that allow users to get specific type the logs they're interested in (e.g., in addition to search path, we may have foreign type checks, etc).

Regardless this thought, we can merge this PR (which is super useful) and create an issue for tracking the follow-up steps. What do you think?

Copy link
Contributor

@mingweishih mingweishih left a comment

Choose a reason for hiding this comment

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

LGTM. Thank! Just have some non-blocking thoughts (see discussion).

@anakrish
Copy link
Contributor Author

anakrish commented Jun 5, 2020

Shouldn't we make this as optional (i.e., a command-line option)?

I am leaning towards not making it an option. oeedger8r always prints messages 'Generating code for Open Enclave SDK', 'Success' (in case of success). Printing the files being processed will add couple more lines to the output; but I don' think most users would care about these additional lines being printed in case of success. On the other hand, those that run into issues would find these lines useful.
What do you think?

For the current status, I think it's fine. I'm just thinking for the long-term, if we consider this as a compiler-like tool, I think it makes sense to make this feature as a command-line option (and we may have many more). For example, a compiler-like tool typically provides -verbose option to display the detailed log. For more complicated ones like gcc/clang, there are multiple options that allow users to get specific type the logs they're interested in (e.g., in addition to search path, we may have foreign type checks, etc).

Regardless this thought, we can merge this PR (which is super useful) and create an issue for tracking the follow-up steps. What do you think?

Yes, long term it makes sense to add verbose option as oeedger8r evolves with more features. Currently it will just be these messages that would be governed by the option; and thus feels not fully justified.
If users do report that these 'Processing path.edl' messages are annoying, we can always introduce the -verbose option.

@anakrish
Copy link
Contributor Author

anakrish commented Jun 5, 2020

Created issue #15

@anakrish anakrish merged commit 9cff8ab into openenclave:master Jun 5, 2020
@anakrish anakrish deleted the ak_searchpath_order branch June 5, 2020 01:10
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.

3 participants