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

Make update #2348

Closed
wants to merge 6 commits into from
Closed

Make update #2348

wants to merge 6 commits into from

Conversation

kfir4444
Copy link
Contributor

@kfir4444 kfir4444 commented Nov 28, 2022

Motivation or Problem

I added a command to make for automating the updates for users that installed rmg from source. This is due to people having issues with forgetting to recompile after pulling. I think this process is a bit more user friendly, especially for the python users that sometimes forget to compile (myself included).

Description of Changes

I added a command to the Makefile for automatic updates, and added an appropriate explanation to the documentations.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #2348 (9de8f02) into main (5149ade) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2348   +/-   ##
=======================================
  Coverage   48.24%   48.24%           
=======================================
  Files         110      110           
  Lines       30626    30626           
  Branches     7986     7986           
=======================================
  Hits        14776    14776           
+ Misses      14306    14305    -1     
- Partials     1544     1545    +1     
Impacted Files Coverage Δ
arkane/kinetics.py 12.24% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Makefile Outdated
update: all
@ echo "Updating RMG-py"
git stash || true
git pull git@github.com:ReactionMechanismGenerator/RMG-Py.git main || git pull https://github.com/ReactionMechanismGenerator/RMG-Py.git main || true
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why pulling twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, either using https or ssh.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

@kfir4444 , thanks for the contribution!
I added a few points for discussion (some where sent as individual emails, sort)

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
This is to make sure that users remember to recompile after making pulling from github.
@ echo "Updating RMG-py"
@ echo "Please remember to stash unwanted changes."
git status
@ read -p "Are there any changes that are not staged for committed? [y|n] " user_input
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be "staged or committed" or "staged for commit"?

@ echo "Please remember to stash unwanted changes."
git status
@ read -p "Are there any changes that are not staged for committed? [y|n] " user_input
if [ "$$user_input" = "N" ] || [ "$$user_input" = "n" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work on your system? because I have the following error when I select y or n:

(rmg_env) blais.ch@FACC02D75Y4MD6T RMG-Py % make update
Updating RMG-py
Please remember to stash unwanted changes.
git status
On branch make_update
Your branch is up to date with 'origin/make_update'.

nothing to commit, working tree clean
Are there any changes that are not staged for committed? [y|n] y
if [ "$user_input" = "N" ] || [ "$user_input" = "n" ]; then
/bin/sh: -c: line 1: syntax error: unexpected end of file
make: *** [update] Error 2

this message appears in both zsh and bash. I assume it has to do with mixing make and bash?

if [ "$$user_input" = "N" ] || [ "$$user_input" = "n" ]; then
git checkout main || true
git pull git@github.com:ReactionMechanismGenerator/RMG-Py.git main || git pull https://github.com/ReactionMechanismGenerator/RMG-Py.git main || true
conda activate rmg_env
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if the environment name weren't hard coded. Mine has a different name, and sometimes I have more than one.
Perhaps instead have a check that RMG (or its dependencies) are available in the current environment, and a warning to activate the right environment if it is not.

You could probably just add check as a dependency for this target? (I think that checks all the python dependencies)

@github-actions
Copy link

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

@github-actions github-actions bot added the stale stale issue/PR as determined by actions bot label Jun 21, 2023
@kfir4444 kfir4444 closed this Jun 22, 2023
@JacksonBurns JacksonBurns deleted the make_update branch June 22, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stale issue/PR as determined by actions bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants