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.

Sign up for GitHub

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

Jump to bottom

Add NRN_VERSION_* test macros for VERBATIM. #1762

Merged
merged 7 commits into from
Apr 11, 2022
Merged

Add NRN_VERSION_* test macros for VERBATIM. #1762

merged 7 commits into from
Apr 11, 2022

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Apr 5, 2022 •
edited
Loading

  • Add some CMake checks to ensure that git describe is consistent with the project version set in the CMake project(...) function.
  • Update that version to 8.2.0 as the current master's parent tag is 8.2.dev.
  • Generate a header containing preprocessor macros for the major, minor and patch version numbers.
  • Add a header that defines various NRN_VERSION_* macros that can be used to make VERBATIM blocks compatible across NEURON versions.

TODO:

@olupton olupton mentioned this pull request Apr 5, 2022
Closed
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #1762 (46628c0) into master (e403c27) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1762      +/-   ##
==========================================
+ Coverage   45.43%   45.49%   +0.06%     
==========================================
  Files         551      551              
  Lines      113149   113149              
==========================================
+ Hits        51405    51476      +71     
+ Misses      61744    61673      -71
Impacted Files Coverage Δ
src/parallel/bbs.cpp 76.27% <0.00%> (+1.69%) ⬆️
src/sparse13/spfactor.c 71.19% <0.00%> (+3.56%) ⬆️
src/parallel/bbssrvmpi.cpp 47.56% <0.00%> (+6.09%) ⬆️
src/parallel/bbsclimpi.cpp 59.19% <0.00%> (+8.62%) ⬆️
src/nrnmpi/bbsmpipack.cpp 92.19% <0.00%> (+10.24%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

nrnhines
nrnhines reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

This all seems sound to me. Do we want to generate installer names using the major.minor.patch info? ...with maybe the hash portion of git describe as a distinction guarantee.

@olupton olupton changed the title Add NEURON_VERSION_* test macros for VERBATIM. Add NRN_VERSION_* test macros for VERBATIM. Apr 6, 2022
@olupton olupton force-pushed the olupton/version-macros branch 3 times, most recently from 904ec5a to b0917f9 Compare April 6, 2022 18:54
This was referenced Apr 7, 2022
Closed
Merged
@olupton
Copy link
Collaborator Author

olupton commented Apr 7, 2022

Do we want to generate installer names using the major.minor.patch info? ...with maybe the hash portion of git describe as a distinction guarantee.

If I understand correctly, these are currently generated from git describe, and this PR is already checking/enforcing consistency, so I am not sure if there is any need to change this 🤔 Do you see a particular advantage? I think for the Python wheels at least it is not a problem that the Python version is a more-unique 8.2.dev-16-deadbeef (or whatever) while the semantic/CMake version is 8.2.0 inside 🤔

@nrnhines
Copy link
Member

nrnhines commented Apr 7, 2022

8.2.dev-16-deadbeef (or whatever) while the semantic/CMake version is 8.2.0

That sufficiently penetrates my fogginess. I guess my model so far is that 8.2.dev is a general indicator in the master and new PRs of ongoing work from bug fix to indefinite future extension.

@olupton olupton marked this pull request as ready for review April 7, 2022 12:42
@olupton
Copy link
Collaborator Author

olupton commented Apr 8, 2022

It occurs to me that in addition to the current content, I should add define one extra macro NRN_VERSION_GTEQ_8_2 (if the version is >=8.2.0, which in practice I think it always will be for versions that include the content of this PR), as then it is possible to write

#ifndef NRN_VERSION_GTEQ_8_2
/* version is < 8.2 */
#endif

in all versions, including ones that predate this PR.
Without this extra change, you have to write something less descriptive and refer to the documentation:

#if !defined(NRN_VERSION_GTEQ)
/* version that didn't include #1762, have to consult the docs to find out that that means < 8.2 */
#endif

@olupton
Copy link
Collaborator Author

olupton commented Apr 8, 2022

Done in 46628c0ed7c68710bbf2bf9300a17c50a9f1db42.

pramodk
pramodk reviewed Apr 11, 2022
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

some minor suggestion otherwise LGTM

CMakeLists.txt Outdated Show resolved Hide resolved
alexsavulescu
alexsavulescu reviewed Apr 11, 2022
View reviewed changes
Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

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

LGTM

@olupton
Add NRN_VERSION_* test macros for VERBATIM.
d1d0429 
- Add some CMake checks to ensure that `git describe` is consistent with
  the project version set in the CMake project(...) function.
- Update that version to 8.2.0 as the current master's parent tag is
  8.2.dev.
- Generate a header containing preprocessor macros for the major, minor
  and patch version numbers.
- Add a header that defines various NRN_VERSION_* macros that can be
  used to make VERBATIM blocks compatible across NEURON versions.
- Update CoreNEURON submodule to pick up an equivalent change.
- Add a test to NEURON that covers both NEURON and CoreNEURON
  implementations of the macros.
@olupton
Add documentation.
ff37a41
@olupton
Update CoreNEURON submodule.
94db8f2
@olupton
Define NRN_VERSION_GTEQ_8_2_0 for NEURON >= 8.2.0
dc3fdc1
@olupton
Use CoreNEURON version instead of compatibility version.
65a4cbc
@olupton
Move git describe parsing to separate file.
23ef82c
@olupton
Copy link
Collaborator Author

olupton commented Apr 11, 2022

The last commit message is not very helpful, as well as moving some CMake code I updated the docs a bit: 23ef82c#diff-6f397e8031813274e13f3fa38bba80cd28fc9c40895a3ac6d225d9dbec7c0359

pramodk
pramodk approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM

@pramodk pramodk merged commit 991863f into master Apr 11, 2022
@pramodk pramodk deleted the olupton/version-macros branch April 11, 2022 19:08
@olupton olupton mentioned this pull request Jun 17, 2022
Merged
@alexsavulescu alexsavulescu mentioned this pull request Jun 29, 2022
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nrnhines nrnhines nrnhines left review comments

@pramodk pramodk pramodk approved these changes

@alexsavulescu alexsavulescu alexsavulescu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@olupton @codecov-commenter @nrnhines @pramodk @alexsavulescu