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

[cling] Startup script support #11262

Merged

Conversation

jiangyilism
Copy link
Contributor

Changes or fixes:

Implement startup script support according to this post:
https://root-forum.cern.ch/t/cling-startup-scripts-support/51358

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@jiangyilism
Copy link
Contributor Author

cling startup scripts are different from rootlogon.C and rootrc .
rootrc is a config file while .clingrc is a regular cling script. cling startup scripts are executed also in invocation of standalone cling binary (not from root interpreter).

Question:

  1. Renaming .cling_profile and .clingrc with suffix .C ?
  2. Rename .clingrc to something else so that .clingrc can be reserved for cling config file in the future (if any)?
    Making it a config file instead of a cling script aligns with rootrc but not with bashrc, zshrc.
  3. Drop .clingrc for now and keep .cling_profile only ? If a cling script can easily tell if it is in interactive mode then keeping only 1 script makes sense. Otherwise it is better to keep both to align with bash and other interpreter inspired by bash design. By easily telling I mean a macro like CLING_INTERACTIVE or something simple that does not access gCling.

@vgvassilev
Copy link
Member

@phsft-bot build!

@jiangyilism, thanks for you PR, can you add some tests for the new functionality under cling/test?

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@jiangyilism
Copy link
Contributor Author

Hi @vgvassilev, cling/test/StratupFile.C is added. However it only covers usage of .cling_profile.C.

To proceed further, I would like to clerify the above decisions first.

Let me summarize again here:

  1. Should we .x execute the startup file(s) or execute it in shebang #! way?
    @eguiraud, @vgvassilev, @Axel-Naumann, what is the designed/documented difference between the 2?
    Will it effect project root (which I have not tried out myself. I am experimenting standalone cling only)
    As far as I read from the web, rootlogon.C and .rootrc are executed in shebang way.
    Their filename-functions rootlogon(), rootrc() are not executed.

    My proposal: Execute startup files in .x way

  2. How should the startup files be named? .cling_profile, .cling_login, .cling_logon like bash and root?

    https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html

    Should we adopt file suffix .C ?

    My proposal: .cling_profile.C to align with rootlogon.C (to be honest I just want the editor syntax highlighting...)

  3. Do we need .cling_profile and .clingrc as the original draft implementation?
    If inside the script we can tell if cling is in interactive mode by a macro like CLING_INTERACTIVE then
    only 1 file is needed.

    My proposal: keep .cling_profile.C as the only startup file. Forget about #define CLING_INTERACTIVE 1 for now.

    I mentioned this support just because bash and other shells/interpreters have it.
    But unless someone wants this feature right now, I prefer not to implement it and
    reserve CLING_INTERACTIVE and .clingrc/.clingrc.C for future design.

@jiangyilism
Copy link
Contributor Author

@Axel-Naumann , @vgvassilev Could you provide some feedback?

@vgvassilev
Copy link
Member

@jiangyilism, thanks for your patch. I am a bit worried about the fact that upon startup cling will execute "random" code. Can you elaborate a little more about your usecase?

I suspect python does something similar, can we model the feature following their approach?

@jiangyilism
Copy link
Contributor Author

My use case is to #include <...> some standard headers I often use. e.g. filesystem and thread.
I will also put namespace fs = std::filesystem; in my startup file.
cling already implicitly includes some standard headers so I think letting users to customize it makes sense.

About "execute random code", is there risk if the executed code/script is in user's home directory or XDG_CONFIG_HOME ?

@vgvassilev
Copy link
Member

My use case is to #include <...> some standard headers I often use. e.g. filesystem and thread. I will also put namespace fs = std::filesystem; in my startup file. cling already implicitly includes some standard headers so I think letting users to customize it makes sense.

Thanks for explaining. It all makes sense.

About "execute random code", is there risk if the executed code/script is in user's home directory or XDG_CONFIG_HOME ?

Well, I guess bash and python suffer from the same issue. As long as we are consistent to their practices it should be no issue.

@vgvassilev
Copy link
Member

cling startup scripts are different from rootlogon.C and rootrc . rootrc is a config file while .clingrc is a regular cling script. cling startup scripts are executed also in invocation of standalone cling binary (not from root interpreter).

Question:

1. Renaming `.cling_profile` and `.clingrc` with suffix `.C` ?

2. Rename  `.clingrc` to something else so that `.clingrc` can be reserved for cling config file in the future (if any)?
   Making it a config file instead of a cling script aligns with rootrc but not with bashrc, zshrc.

3. Drop `.clingrc`  for now and keep `.cling_profile` only ?  If a cling script can easily tell if it is in interactive mode then keeping only 1 script makes sense. Otherwise it is better to keep both to align with bash and other interpreter inspired by bash design. By easily telling I mean a macro like `CLING_INTERACTIVE` or something simple that does not access `gCling`.

How about a ~/.cling.d folder where we glob all files and execute?

@jiangyilism
Copy link
Contributor Author

How about a ~/.cling.d folder where we glob all files and execute?

Sounds good to me too.

I will enumerate .C files in .cling.d/ (if dir exists) with llvm::vfs::directory_iterator like CompilerInstance.cpp. So the enumeration does not guarantee specific orderings.

Search order of .cling.d/ is still

  1. ${CLING_HOME} envvar
  2. ${XDG_CONFIG_HOME}/cling/
  3. ${HOME}/.config/cling/
  4. ${HOME}/

Does that look good to you?

@Axel-Naumann
Copy link
Member

Apologies for not reacting - this sounds terrific! Could you update the PR?

@jiangyilism
Copy link
Contributor Author

So the enumeration does not guarantee specific orderings.

Updated. However the behavior is slightly different: the startup files are sorted after enumeration so the execution order is deterministic.

Copy link
Member

@Axel-Naumann Axel-Naumann left a comment

Choose a reason for hiding this comment

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

LGTM, please just address the spelling mistake and I'll merge!

#include <fstream>
#include <vector>
#include <iostream>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

interpreter/cling/tools/driver/cling.cpp Outdated Show resolved Hide resolved
@Axel-Naumann
Copy link
Member

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

Before showing command promt or executing scripts from argv,
enumerate .C files in the directory ${CLING_HOME}/.cling.d/ in alphabetic
order then .x them.

Search order of ${CLING_HOME}:

1. ${CLING_HOME} envvar
2. ${XDG_CONFIG_HOME}/cling/
3. ${HOME}/.config/cling/
4. ${HOME}/
@phsft-bot
Copy link
Collaborator

Build failed on mac11/cxx14.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@vgvassilev
Copy link
Member

@jiangyilism I was wondering what's the status of this PR?

@jiangyilism
Copy link
Contributor Author

@jiangyilism I was wondering what's the status of this PR?

There is no pending issue as far as I know. The mac11/cxx14 test fail above is due to (flaky?) timeout so it seems irrelevant to this PR.

@vgvassilev
Copy link
Member

@Axel-Naumann, should we merge this then?

@github-actions
Copy link

Test Results

       11 files         11 suites   2d 5h 55m 22s ⏱️
  2 469 tests   2 460 ✔️ 0 💤   9
25 472 runs  25 461 ✔️ 0 💤 11

For more details on these failures, see this check.

Results for commit f816fae.

@Axel-Naumann Axel-Naumann merged commit fd991ce into root-project:master Jun 16, 2023
11 of 15 checks passed
@Axel-Naumann
Copy link
Member

Thanks for your very nice contribution!

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.

None yet

4 participants