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

Prepare for official release #303

Merged
merged 12 commits into from
Oct 30, 2018
Merged

Prepare for official release #303

merged 12 commits into from
Oct 30, 2018

Conversation

rs028
Copy link
Collaborator

@rs028 rs028 commented Jul 12, 2018

This includes all the final changes required to prepare a new release.

NB: need to merge PR #266 first.

@codecov-io
Copy link

codecov-io commented Jul 12, 2018

Codecov Report

Merging #303 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #303   +/-   ##
=======================================
  Coverage   90.41%   90.41%           
=======================================
  Files          15       15           
  Lines        2108     2108           
=======================================
  Hits         1906     1906           
  Misses        202      202
Flag Coverage Δ
#build 65.29% <100%> (ø) ⬆️
#tests 89.75% <100%> (ø) ⬆️
#unittests 34.91% <100%> (ø) ⬆️
Impacted Files Coverage Δ
src/atchem2.f90 91.49% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67fab2e...685d606. Read the comment docs.

@rs028 rs028 mentioned this pull request Jul 13, 2018
@rs028
Copy link
Collaborator Author

rs028 commented Jul 13, 2018

@spco I think I can dedicate some time in the next couple of months to the wiki pages, but not to writing more code, so this seems a good point to stop and make a new release. Or is there any other issue that you will be able to address right now?

@rs028 rs028 requested a review from spco July 13, 2018 15:44
Copy link
Collaborator

@spco spco left a comment

Choose a reason for hiding this comment

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

Just the one comment, looks good beyond that. I think the reworking of README.md is very useful.

README.md Outdated
The `src/` directory contains the source files of AtChem2. The `tools/` directory contains Python and shell scripts to build the AtChem2 executable for a given chemical mechanism and configuration. The `travis/` directory contains the _testsuite_ files. The remaining directories are empty, but provide a default location for the input, output and configuration files.
- `mcm/` contains data files related to specific versions of the MCM.
- `model/` contains the chemical mechanism (in FACSIMILE format) and directories for the model configuration, the model constraints and the model output
- `obj` contains the files generated by the Fortran compiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be obj/ for consistency.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 16, 2018

Somehow I broke it. But it works on my system.

@spco
Copy link
Collaborator

spco commented Jul 16, 2018

You've removed mkdir -p $1, so when you cd $1 it doesn't exist. Then install_fruit.sh installs it into (probably) $HOME, and things fall over when they look for it.

I'm also surprised that removing rvm install ruby-2.4.2 doesn't trip up the Mac version. Perhaps it's just my local system that needed the upgrade, not the Travis build.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 16, 2018

I am trying to keep the behaviour consistent for all the install scripts.
If, for example, all the libraries are in ~/libraries/:

install_numdiff.sh ~/libraries/

creates a ~/libraries/numdiff-5.8.1/ directory and then make; make install create a ~/libraries/numdiff/ directory which is where the compiled library is located.

Likewise:

install_openlibm.sh ~/libraries/

installs openlibm in ~/libraries/openlibm-0.4.1/

Then:

install_fruit.sh ~/libraries/

shoud create a ~/libraries/fruit-3.4.3/ after unzip. In other words the argument for all these scripts is the base directory for all the dependencies. Does it make sense?

EDIT: tweaked the travis.yml and Makefile, should be fine now.

@spco
Copy link
Collaborator

spco commented Jul 17, 2018

That looks like a very sensible change!

.gitignore Outdated
@@ -1,10 +1,6 @@
# Ignore object files in this directory
*.o
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think these are not needed anymore, because they go into the obj/ directory. Right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe you're right

fruit_driver.exe
*.xml
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the xml file ends up in the main directory. Also are *_gen.f90 and Makefile in here?

@spco
Copy link
Collaborator

spco commented Jul 30, 2018

Don't worry about codcov saying you're failing the codecov/projects/tests category. But moving fruit_generator.rb out of travis/unit_tests is sure to break the Travis build as above!

@spco
Copy link
Collaborator

spco commented Jul 30, 2018

I'm also thinking it would be good to split this PR up - it contains bits we'd only want to put in when we decide to release v1.1, like the changelog, but it also contains a number of other improvements that would be useful to include regardless of when we release. Would you consider moving those parts out so we can merge them sooner?

@rs028
Copy link
Collaborator Author

rs028 commented Jul 30, 2018

I was trying to use for fruit_generator.rb the same approach as for Makefile to simplify installation, but I suppose it is not possibile at the moment.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 30, 2018

Yes, I am okay splitting this up. The only changes that cannot be merged at this stage are the changelog and the v1.0-dev1 to v1.1` strings.
That being said, I am not sure how to do that :)

@spco
Copy link
Collaborator

spco commented Jul 30, 2018

My advise would be to copy this branch (locally) to another (when you're at the head of the branch, do git checkout -b non_release) and then edit that branch until you're happy, then push to github with a new PR. For the editing step, if the commits are such that you just want to remove a couple (e.g. remove 2 that add the changelog and the change to v1.1 in the outputs) then you can do git rebase -i master while on non_release branch, and then delete the lines corresponding to the commits you want to remove.

Just make sure you're happy with that branch before pushing. Once that is all done, we can come back and remove the unnecessary commits from this branch with anothe rebase, but that will then require a force-push to update this PR. I'd hold off on that until you've got the non_release branch sorted.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 30, 2018

Should I close this and open a new one?

@spco
Copy link
Collaborator

spco commented Jul 31, 2018

What remains to be added from this? I think it's neater if you continue to use this PR but either (1) add reverting commits to undo the now-redundant parts, and squash it all into one commit when merging, or (2) do a local rebase to remove now-redundant parts and force-push. Either is preferable to opening a new PR, as that makes the history hard to follow - nothing wrong with having a messy PR here if that's what actually happened!

@rs028
Copy link
Collaborator Author

rs028 commented Jul 31, 2018

So I tried to do this, but when I try to git push --set-upstream origin release this is what I get:

To [email protected]:rs028/AtChem2.git
 ! [rejected]        release -> release (non-fast-forward)
error: failed to push some refs to '[email protected]:rs028/AtChem2.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.

and I am not sure what it means. It rebased fine (I think).

@spco
Copy link
Collaborator

spco commented Jul 31, 2018

So you shouldn't need to reset the upstream - just use git push. Are you saying that you've rebased on top of the latest merged changes and are now trying to push straight after that, or have you also edited the branch? Either way, the fact is that you are re-writing history (because the branch is now being based somewhere different to previously), so the remote (GitHub) is complaining. Using git push --force will ensure it works, but please make sure your local history is definitely correct before doing so.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 31, 2018

No, not really. I messed up rebasing, so I deleted the release branch and created a new branch from master called release. I added the changelog file and modified the version number and when I try to puch I got that message. Probably it was a bad idea to proceed that way :(

The log looks like this:

* f2b4b24 (HEAD, release) Set version number
* 9ab2641 Add changelog file
* cc854d5 (origin/master, master) Pre-release (#309)
* 9f27e84 Add travis/unit_tests to the paths codecov monitors for coverage.
* d9c58d4 Add tests for convertRHtoH2O(), and use parts-per-unit rather thant parts-per-million to reduce numeri
* 7fdaabc Remove unnecessary dependency in tools/setup_atchem.py
* 70dabd9 Correct dec (#266)
* 009b73a Convert .fac so that all constants in mechanism definition use _DP type.
* 77635ec Change remaining non-generated constants to use e+08_DP instead of D+08 form. Also change generated li
* 9915212 (upstream/master, upstream/HEAD) Move unittest build to base Makefile. (#300)
* 9d81e87 Add test_calcAirDensity unit test. (#298)
* c9e8a65 Use FRUIT for unit testing (#292)
* ea5cfac Update MCM related files (#290)

@spco
Copy link
Collaborator

spco commented Jul 31, 2018

Ah right. Best thing is to force-push then - that will delete the old history and entirely replace it.

@rs028
Copy link
Collaborator Author

rs028 commented Jul 31, 2018

all right, back on track!

@rs028 rs028 force-pushed the release branch 2 times, most recently from b950f90 to c5466fa Compare August 6, 2018 16:09
@rs028
Copy link
Collaborator Author

rs028 commented Oct 29, 2018

@spco can you quickly review this and make sure I did not forget anything or made any mistakes?
I want to try installing everything from scratch to see if there is any last minute fixes to do, after this is merged.

@spco
Copy link
Collaborator

spco commented Oct 30, 2018

I've just triggered a rebuild as the unit tests coverage hadn't been detected correctly. It all looks good from my perspective.

@rs028
Copy link
Collaborator Author

rs028 commented Oct 30, 2018

ah okay. how do you trigger a rebuild?

@spco
Copy link
Collaborator

spco commented Oct 30, 2018

Go to the Travis CI build via the tick/cross on the commit, and on the right there's a "Restart build" button.

@rs028 rs028 merged commit 9e0526c into AtChem:master Oct 30, 2018
@spco
Copy link
Collaborator

spco commented Oct 30, 2018

🍾

@rs028 rs028 deleted the release branch October 30, 2018 10:27
@rs028
Copy link
Collaborator Author

rs028 commented Oct 30, 2018

mmh... why if I do git status on my master branch I am getting:

# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   travis/tests/full/model/configuration/full.fac
#	modified:   travis/tests/short/model/configuration/short.fac

basically for all the fac files in travis/tests?

@spco
Copy link
Collaborator

spco commented Oct 30, 2018

Is this before or after git pull upstream master? Can you show me the output of git log --oneline --branches --graph --decorate?

@rs028
Copy link
Collaborator Author

rs028 commented Oct 30, 2018

I got the warning when I switched from the release branch to master. I thought it would be fixed by doing git pull upstream master, but it is still there. I am going to delete and reinstall everything from zero so it is not a big deal, but I want to be sure I did not mess up the merge. The log file is below.

 9e0526c (HEAD, origin/master, master) Prepare for official release (#303)
* 67fab2e Always use 'press' and 'blheight' for the names of the environment variables. This matches currently used input config iles.
* fcfb2a1 Fix missing indent and format tests from Travis CI (#353)
* 2f90e57 Add functionality to tools/fix_mechanism_fac.py to handle damaged files without carriage returns. (#352)
* 9da7615 Change the outputs to loss/productionRates.output, as the previous values were meaningless. (#351)
* ea3c795 Add handling of type definitions to the fix_indent.py script.
* 8638ab4 Rename instananeousRates globally to reactionRates.
* 127c505 Replace generic complex variables with vector q (#347)
* 7170899 tools/mech_converter.py no longer creates ro2-rates.f90, which needs adding in at compilation time. Instead, it creates mechanism.ro2, which is read in at run-time. Thi
* b3ed2ba Move call to ro2sum() out of generated file to normal source file.
* c5e0bcb Remove the unnecessary variable size_of_j from the photolysis_rates_mod - keep it as an input variable to the function allocate_photolysis_j().
* 738bf50 Reduce the number of module-wide imports, and add a couple of TODOs.
* 137517b Remove perl search for single-precision constants.

@spco
Copy link
Collaborator

spco commented Oct 30, 2018

Hmm, there's nothing there to suggest anything is wrong. You could always try git adding them all - it may be that doing so removes them from that untracked category, but it's all up to data anyway. Not sure. The repo looks right, full.fac on master on GitHub hasn't changed in 4 months. Not sure what the issue is, but I think it's only at your end :)

@rs028
Copy link
Collaborator Author

rs028 commented Oct 30, 2018

okay then :)

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