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

Updated Makefile and docs for fedsd utility #262

Merged
merged 18 commits into from
Sep 1, 2023
Merged

Updated Makefile and docs for fedsd utility #262

merged 18 commits into from
Sep 1, 2023

Conversation

ChadliaJerad
Copy link
Collaborator

When installing the tracing executables to /usr/local/bin, fedsd will not work, because it is operating on a dangling symlink.

This PR provides a fix.

@ChadliaJerad
Copy link
Collaborator Author

BTW, launch-fedsd.sh script is broken, since it relied on the paths to fedsd.py and trace_to_csv executable being known.
Will work on this.

@ChadliaJerad
Copy link
Collaborator Author

It works this way, but seems more like a hack...

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

One problem with the fix to the Makefile is that now a file in /usr/local/bin is a symbolic link into a user's clone of the repo. This seems problematic to me. Would it work instead to copy launch-fedsd.sh into /usr/local/bin (or wherever is specified with INSTALL_PREFIX)?

Also, for the other changes that try to prepend a path to trace_to_csv using an INSTALL_PREFIX argument to fedsd, this seems awkward. If trace_to_csv is in the user's PATH, it seems like it would not be necessary. Can we just check to see whether trace_to_csv is in the user's path and issue a friendly error message if not?

@erlingrj
Copy link
Collaborator

I am sorry about this. I didnt test the fedsd utility. I think I agree with Edward, it is not unreasonable to demand that for fedsd to work, certain executables must be on the PATH.

@erlingrj
Copy link
Collaborator

erlingrj commented Aug 19, 2023

What is, btw, the reason for having this launch shell launch-fedsd.sh script wrapping a Python script fedsd.py instead of making the Python script itself executable and putting it on the PATH?

@erlingrj
Copy link
Collaborator

The Zephyr test failure is fixed by updating LF ref here: #263

@lhstrh
Copy link
Member

lhstrh commented Aug 19, 2023

What is, btw, the reason for having this launch shell launch-fedsd.sh script wrapping a Python script fedsd.py instead of making the Python script itself executable and putting it on the PATH?

No need for the launch script, IMO. Let make install copy the Python script and be done with it? Make sure it allows for specifying an installation prefix...

@edwardalee
Copy link
Contributor

What is, btw, the reason for having this launch shell launch-fedsd.sh script wrapping a Python script fedsd.py instead of making the Python script itself executable and putting it on the PATH?

No need for the launch script, IMO. Let make install copy the Python script and be done with it? Make sure it allows for specifying an installation prefix...

Can a python program be made executable? I was unaware of that... How does the OS know which python to invoke?

@lhstrh
Copy link
Member

lhstrh commented Aug 19, 2023

Can a python program be made executable? I was unaware of that... How does the OS know which python to invoke?

https://realpython.com/python-shebang/

@ChadliaJerad
Copy link
Collaborator Author

Just pushed a fix, where launch-fedsd.sh is no longer needed.
I have a question though. Since executing Python Scripts with a shebang under Windows requires an extra configuration effort, where shall we mention this?
I thought this is the reason behind using the launch scripts...

A detail worth mentioning in this update is that one does not need saying fedsd *.lft anymore. Calling fedsd without arguments will automatically search .lft files. At the same time, it is still possible to operate of specific trace files that are given as arguments.

Regarding fedsd_helper.py, it is also copied with fedsd.py. Should we simply keep one file by putting the contents of the first in the second?

@lhstrh
Copy link
Member

lhstrh commented Aug 22, 2023

Just pushed a fix, where launch-fedsd.sh is no longer needed. I have a question though. Since executing Python Scripts with a shebang under Windows requires an extra configuration effort, where shall we mention this? I thought this is the reason behind using the launch scripts...

What configuration effort? On Windows, a bash script must be executed in WSL, and in WSL running a script with a #! doesn't require any configuration. The file must be executable (+x) though.

@ChadliaJerad
Copy link
Collaborator Author

What configuration effort? On Windows, a bash script must be executed in WSL, and in WSL running a script with a #! doesn't require any configuration. The file must be executable (+x) though.

Yes yes, you're right! Same for Cygwin.
Actually, I don't have a decent understanding of LF support on Windows platform...

util/tracing/Makefile Outdated Show resolved Hide resolved
util/tracing/visualization/fedsd.py Outdated Show resolved Hide resolved
util/tracing/visualization/fedsd.py Outdated Show resolved Hide resolved
util/tracing/Makefile Outdated Show resolved Hide resolved
util/tracing/Makefile Show resolved Hide resolved
util/tracing/visualization/fedsd_helper.py Outdated Show resolved Hide resolved
util/tracing/visualization/fedsd_helper.py Outdated Show resolved Hide resolved
util/tracing/visualization/fedsd_helper.py Outdated Show resolved Hide resolved
util/tracing/visualization/fedsd_helper.py Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Aug 22, 2023

What configuration effort? On Windows, a bash script must be executed in WSL, and in WSL running a script with a #! doesn't require any configuration. The file must be executable (+x) though.

Yes yes, you're right! Same for Cygwin. Actually, I don't have a decent understanding of LF support on Windows platform...

Since the introduction of WSL, there is a whole lot less to worry about. We can just tell Windows users to use WSL...

@lhstrh
Copy link
Member

lhstrh commented Aug 22, 2023

To build confidence in multi-platform support, just add some tests in CI and let them run them on all platforms. I don't believe there are currently any tests, so please add those.

util/tracing/Makefile Outdated Show resolved Hide resolved
@ChadliaJerad ChadliaJerad mentioned this pull request Aug 28, 2023
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Modulo suggestions, this looks good, but there are still no tests, so we don't have any assurance that this works and will keep working...

util/tracing/Makefile Outdated Show resolved Hide resolved
util/tracing/Makefile Outdated Show resolved Hide resolved
util/tracing/Makefile Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Sep 1, 2023

Since it appears that what's in main is broken and this is a fix, let's merge this. But we do need tests.

@lhstrh lhstrh self-requested a review September 1, 2023 05:39
@lhstrh lhstrh dismissed edwardalee’s stale review September 1, 2023 05:40

Comment has been addressed.

@lhstrh lhstrh changed the title Fix fedsd symbolic link Updated Makefile and docs for fedsd utility Sep 1, 2023
@lhstrh lhstrh merged commit 09b75ed into main Sep 1, 2023
21 of 28 checks passed
@lhstrh lhstrh deleted the fix-fedsd-symlink branch September 1, 2023 05:48
@lhstrh lhstrh added the bugfix label Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants