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

Migrate pt-run to a complete Lua frontend #21

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Conversation

singul4ri7y
Copy link
Collaborator

@singul4ri7y singul4ri7y commented Aug 11, 2024

Changelog:

  • Migrate pt-run to pt-lua, which acts like a complete Lua frontend with our custom traceback function being the default one.
  • Add -isystem flag in CPPFLAGS of Makefile, because if Lua and Lua internals are installed in the same machine, sometimes compiler finds it troublesome regarding which Include directory to use, /usr for Lua or /usr/local for Lua internals. Maybe don't need this if Lua internals always copes up with the upstream Lua version, but I would like to keep it just incase. Noteworthy to mention, using -I won't do.
  • Update tracebacks tests, removing the change directory to each tests. Makes the tracebacks inconsistent (module traces having path prefix where Lua traces does not) in the tests and also unnecessary now that we don't have separate Makefiles to take care of.

Resolves #13

@hugomg
Copy link
Member

hugomg commented Aug 11, 2024

Since renaming pt-run is an API change that will break Pallene builds, can we implement pallene-tracer version numbers before we merge this?

@singul4ri7y
Copy link
Collaborator Author

Let's do it after we are done stabilizing it? We are almost done tbh

@singul4ri7y singul4ri7y marked this pull request as draft August 11, 2024 16:10
@singul4ri7y
Copy link
Collaborator Author

singul4ri7y commented Aug 11, 2024

I think we better merge this after Lua internals and Pallene is bumped up to Lua 5.4.7.

Makefile Outdated Show resolved Hide resolved
pt-lua.c Outdated Show resolved Hide resolved
pt-lua.c Outdated Show resolved Hide resolved
pt-lua.c Outdated Show resolved Hide resolved
pt-lua.c Outdated Show resolved Hide resolved
pt-lua.c Show resolved Hide resolved
pt-lua.c Show resolved Hide resolved
@hugomg
Copy link
Member

hugomg commented Aug 11, 2024

Agreed on merging this after 5.4.7

As for the version numbers, I suppose we might as well do it now? We already had unrelated Pallene PRs breaking because of ptracer API changes.

@singul4ri7y
Copy link
Collaborator Author

Yes, let's do version tagging after this merge. I will come up with commits fixing the nits tomorrow. We won't be releasing on this tag though. Also it would be wise to mark the tag as alpha if possible.

@hugomg
Copy link
Member

hugomg commented Aug 11, 2024

The problem is that since we switched Pallene to use the separate pallene tracer repo, any changes we make here break the CI on all the Pallene PRs, including that one that Gabriel is working on. Can we please do the version numbers first?

@singul4ri7y
Copy link
Collaborator Author

Fine by me. But it would have been great if we had atleast pt-lua. If version numbers can wait one more day, I can complete this PR and then can do version numbering.

@singul4ri7y singul4ri7y marked this pull request as ready for review August 12, 2024 11:46
Copy link
Member

@hugomg hugomg left a comment

Choose a reason for hiding this comment

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

I'm still not convinced about the -isystem. I have never seen that in a makefile. The -I ought to have been sufficient? What do you mean by headers are not properly resolved? Have you tested using LUA_PREFIX=/usr/local?

pt-lua.c Outdated Show resolved Hide resolved
pt-lua.c Outdated Show resolved Hide resolved
@singul4ri7y
Copy link
Collaborator Author

Have I tested with LUA_PREFIX=/usr/local? Yes.

It does not work, simple as that. There is nothing to be convinced about. the -I prefix supposed to append the dir to the leftmost (or topmost) header searching path string, but clearly the verbose logging I showed you earlier does not follow that. I also have double checked for multiple times, it does not work.

@hugomg
Copy link
Member

hugomg commented Aug 13, 2024

In the verbose logging, you use -I/usr/include and -L/usr/lib compilation flags. Why wasn't it /usr/local?

@singul4ri7y
Copy link
Collaborator Author

@hugomg Again, that's not the point. I used it because in my case if I wanted to compile pt-lua against my system Lua. It was still grabbing Lua internal headers from /usr/local/include, the -I was not working. So, I had to switch to -isystem for that. Now, the situation can be very well vice versa, say you have a backdated Lua in your system that you want to compile against and a newer version of Lua is installed in /usr/local.

By the way, using /usr/local was still giving me errors, because as usual with -isystem the /usr/local was getting resolved first.

@singul4ri7y
Copy link
Collaborator Author

I highly suggest that you try the -isystem flag yourself. Also try having Lua internals 5.4.6 with you, that would make my point aloud.

@hugomg
Copy link
Member

hugomg commented Aug 13, 2024

That is not a configuration that we have to support. The C header file include system is not designed to handle using ptracer from usr/local but then not using the Lua from /usr/local. If the user really wants to do that, they should install lua-internals and ptracer to different prefixes.

@singul4ri7y
Copy link
Collaborator Author

I am not convinced. The -I flag is not clearly working as the documentation suggested. Moreover, users won't be using our Makefile to build other projects, so I think the flag is fine. If the -I flag works as intended, we may change it back.

As for prefixes. I am pretty much sure users won't like to make their life a hassle by installing to a different prefix and adding it to their path just for a single goddang flag, which is not total taboo.

Copy link
Member

@hugomg hugomg left a comment

Choose a reason for hiding this comment

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

Consider adding version numbers before merging this.

@singul4ri7y
Copy link
Collaborator Author

Should we create version tag after merging this? Because regardless, we have to make changes to Pallene with another PR.

@hugomg
Copy link
Member

hugomg commented Aug 14, 2024 via email

@singul4ri7y singul4ri7y merged commit a241fd3 into main Aug 14, 2024
1 check passed
@singul4ri7y singul4ri7y deleted the pt-run-2-pt-lua branch August 17, 2024 15:25
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.

feature request: turn pt-run into pt-lua?
2 participants