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

Adding tracing of SConscript calls #4438

Closed
wants to merge 4 commits into from
Closed

Adding tracing of SConscript calls #4438

wants to merge 4 commits into from

Conversation

StenGruener
Copy link
Contributor

@StenGruener StenGruener commented Oct 27, 2023

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

Sorry to be so ignorant with respect to checklist, but I wanted to make sure it this PR is able to be accepted at all. I am coming from the discussion here: https://stackoverflow.com/questions/76520575/scons-overriding-sconscript-function-to-get-a-list-all-loaded-sconscripts . So is there hope that our approach can go into the main line?

@mwichmann
Copy link
Collaborator

mwichmann commented Oct 27, 2023

Thanks for this. SCons has a historical bias against adding more meaningful environment variables, so that may be an impediment to this precise implementation, but will wait for further feedback. (I do have an experimental branch somewhere that adds a CLI option, as a possible alternative). Whatever the solution, we'll also need to work up some testing to show it works as expected. That doesn't have to happen until the approach is approved.

@bdbaddog
Copy link
Contributor

Yes. I'd use a --debug= flag to enable this, that'd be mergable.

(Also probably helpful to join our discord server to speed the process of getting your PR in shape for merging)

@StenGruener
Copy link
Contributor Author

StenGruener commented Oct 28, 2023

Thank you gentlemen!
I have added a --debug=sconscript-trace flag and a unit-test. Could you please kindly advice me:

  • Whether the folder for the unit tests ist okay.
  • How to document? I have seen the xml files but am a bit afraid in patching them without tooling (sorry for my ignorance - have never seen scons from inside before).

P.S.: Discord blocked on my laptop. Is it really needed to get an app?

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 28, 2023

You can connect to discord in your web browser.

I'd prefer --debug=sconscript than --debug=sconscript-trace

Do you have access to a mac or linux machine?
It's much easier to build the documentation on that than windows.

@StenGruener
Copy link
Contributor Author

StenGruener commented Oct 28, 2023 via email

@bdbaddog
Copy link
Contributor

I got Linux, doing my round of rtfm

________________________________ Von: William Deegan @.> Gesendet: Saturday, October 28, 2023 8:56:32 PM An: SCons/scons @.> Cc: Sten Gruener @.>; Author @.> Betreff: Re: [SCons/scons] Adding tracing of scons script calls (PR #4438) BeSecure! This email comes from outside of ABB. Make sure you verify the sender before clicking any links or downloading/opening attachments. If this email looks suspicious, report it by clicking 'Report Phishing' button in Outlook or raising a ticket on MyIS. You can connect to discord in your web browser. I'd prefer --debug=sconscript than --debug=sconscript-trace Do you have access to a mac or linux machine, much easier to build the documentation on that than windows.
-Bill — Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/SCons/scons/pull/4438*issuecomment-1783897507__;Iw!!NLW3fF9v!LFjV1ogn6cxdfobi_7Q55U8TNfzUaD8g6GEQoBcxptgKP95091XfNbyn6fSWbDPYJi50xtHEU_D-tmPz8SUS7uZVuuM$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AQUNYPNA2E7B4J3OZUE7GE3YBVIOBAVCNFSM6AAAAAA6S274DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBTHA4TONJQG4__;!!NLW3fF9v!LFjV1ogn6cxdfobi_7Q55U8TNfzUaD8g6GEQoBcxptgKP95091XfNbyn6fSWbDPYJi50xtHEU_D-tmPz8SUS7Apc4fo$. You are receiving this because you authored the thread.Message ID: @.***>

Which distro. There's a script in the repo for setting up ubuntu. bin/scons_dev_master.py building docs testing should get you set up. It may be slightly out of date depending how new your distro is.

@StenGruener
Copy link
Contributor Author

okay, i got stuff installed following git actions workflow

  • key renamed to sconscript
  • info added to the manpage part

@StenGruener StenGruener changed the title Adding tracing of scons script calls Adding tracing of SConscript calls Oct 28, 2023
@bdbaddog
Copy link
Contributor

I have a handful of fixes and changes and reorganizations to this code to make it mergable.
Can you allow project maintainers to push to your repo?
Or merge from https://github.com/bdbaddog/scons/tree/debug-sconscript

git remote add bdbaddog https://github.com/bdbaddog/scons
git fetch bdbaddog
git merge bdbaddog/debug-sconscript

I think should do it.

Here's a summary of the changes:

  1. In per release section of CHANGES.txt the entries should be sorted by contributors last name.
  2. Changed entering/exiting text to match style of other similar messages
  3. moved test to options/debug-sconscript as that's where the other option tests are.
  4. Fixed copyright and such to match current test templated header

@bdbaddog
Copy link
Contributor

@StenGruener
Actually I just created a separate PR #4439 which is my changes of this PR.
Please take a look and comment there.

I'm ready to merge my alternate PR unless you have any strong objections?

@StenGruener
Copy link
Contributor Author

Lets continue in #4439

@bdbaddog
Copy link
Contributor

@StenGruener - for your next PR, we kinda prefer if you create a branch and submit from that rather than submitting PR from your master branch.
Here's our git workflow doc. https://github.com/SCons/scons/wiki/GitWorkflows

@bdbaddog
Copy link
Contributor

Thanks for the PR and new functionality! :)

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