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

just adopt a task runner #1380

Merged
merged 31 commits into from
Aug 31, 2024
Merged

just adopt a task runner #1380

merged 31 commits into from
Aug 31, 2024

Conversation

tomeichlersmith
Copy link
Member

@tomeichlersmith tomeichlersmith commented Jul 31, 2024

I am updating ldmx-sw by including a justfile and rewriting the README to point folks to denv and just. The ldmx-env.sh script is left in place and is only changed to include a deprecation notice if a user sources it and does not have both denv and just installed. (If both are available, then ldmx is just an alias for just and the justfile recipes are used instead of the ldmx-env.sh bash functions.)

The best way to show why I chose this workflow is probably to show some examples assuming denv and just are installed.

New Clone

git clone --recursive [email protected]:LDMX-Software/ldmx-sw.git
cd ldmx-sw
just init

Default Build

just configure build

From somewhere else

just path/to/ldmx-sw/build test

Pretty Output
image

What are the issues that this addresses?

This resolves #1377 directly and resolves #1248 indirectly (via denv).

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.
  • Add rootbrowse recipe and root
  • Update doc website Separate users from developers ldmx-software.github.io#35
    • As the re-written README points out, I think using denv and the production images allows many folks who are just using ldmx-sw to do studies to avoid compiling ldmx-sw themselves. I think this is a nice workflow since it both pins the version of ldmx-sw of the study and allows users to jump right in to a physics study without needing to run the compilation. This is a workflow change though and so I'd want to update the doc website to reflect this suggestion.
  • Stabilize denv and create LDMX-Software fork

After @tvami 's first review...

  • Have an ldmx symlink to denv to allow folks used to the ldmx program to continue using it after transition
  • Flesh out README (and doc website) with example snippets of installing and enabling tab complete
  • Connect tab complete of just to ldmx alias within ldmx-env.sh
  • Undo chmod on TrigScint script

🛑 Won't Do 🛑

  • support /cvmfs/unpacked.cern.ch in environment script #1252 could be done within these recipes but I think it is too complicated to try to do on this first pass
  • implement a from-ldmx-env recipe. Thought about it, but denv pretty much requires the ldmx/dev image to be one of the newest available and other updates to ldmx-sw deprecate non-v4 images, so encouraging updating is a good way to go.
  • put in python formatting right now. This is a bigger discussion that we can add later.
  • fully test all command orderings. Further bugs can be patched as we encounter them, going to focus on making sure new developers can understand how to get started.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@tvami
Copy link
Member

tvami commented Aug 30, 2024

Another feature I'm missing is the autocomplete, with earlier ldmx comp<tab> autocomplete to ldmx compile, now that's gone, can we add it?

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

I tested most of the features, both on Linux and Mac. My most important ask is the autocomplete, the rest I could imagine life w/o....

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
TrigScint/exampleConfigs/runTestBeamHitReco.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
justfile Show resolved Hide resolved
@tomeichlersmith
Copy link
Member Author

You can enable just's autocomplete https://just.systems/man/en/chapter_70.html

and then you need to attach these autocomplete functions to the ldmx alias as well https://just.systems/man/en/chapter_69.html?highlight=.j#shell-alias

I can put the second one into the ldmx-env.sh but, similar to the installation of just itself, tab-completion depends on the shell being used and where the user would like to install things. (What I do on my clusters which I could put into the README as an example as well is

just --completions bash >> ~/.local/share/bash-completion/completions/just

so that they are auto-loaded by bash when needed.)

@tvami
Copy link
Member

tvami commented Aug 30, 2024

What I do on my clusters which I could put into the README as an example as well is

OK this works for me, I'm happy now!

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

Major changes requested are done (TS config file is still there, but I guess that's ok)

Edit: All good now! Thanks Tom!

patch check calls to be quiet and not run command

partition justfile to have mimics of ldmx at bottom

don't re-print just --list call

fix mimics to only be one arg for image or dir

draft a format function"
more notes and soup up some of the commands

format requires space between recipes

add some printouts with ansi coloring

use a tempfile for listing c++ files to format

add clean and test recipes
remove format short alias
also label features with just version requirement
start reworking readme

tighten up listing of git requirement

update links in readme

comment about necessity of just

add `just init` to list of startup commands for developers
@tomeichlersmith tomeichlersmith force-pushed the 1377-just-adopt-a-task-runner branch from 7ad22ba to 57ffa7c Compare August 30, 2024 21:12
@tomeichlersmith tomeichlersmith merged commit 605aa50 into trunk Aug 31, 2024
2 checks passed
@tomeichlersmith tomeichlersmith deleted the 1377-just-adopt-a-task-runner branch August 31, 2024 17:47
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.

just adopt a task runner add apptainer explicitly to the ldmx-env.sh script
2 participants