Skip to content

Add NRN_VERSION_* test macros for VERBATIM.#1762

Merged
pramodk merged 7 commits into
masterfrom
olupton/version-macros
Apr 11, 2022
Merged

Add NRN_VERSION_* test macros for VERBATIM.#1762
pramodk merged 7 commits into
masterfrom
olupton/version-macros

Conversation

@olupton

@olupton olupton commented Apr 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator
  • 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

codecov-commenter commented Apr 5, 2022 •
edited
Loading

Copy link
Copy Markdown

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

@nrnhines nrnhines left a comment

Copy link
Copy Markdown
Member

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

olupton commented Apr 7, 2022

Copy link
Copy Markdown
Collaborator Author

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 🤔

@olupton olupton force-pushed the olupton/version-macros branch from ef5c20e to b4ffe5c Compare April 7, 2022 07:42
@nrnhines

nrnhines commented Apr 7, 2022

Copy link
Copy Markdown
Member

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

olupton commented Apr 8, 2022

Copy link
Copy Markdown
Collaborator Author

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

olupton commented Apr 8, 2022

Copy link
Copy Markdown
Collaborator Author

Done in 46628c0ed7c68710bbf2bf9300a17c50a9f1db42.

pramodk
pramodk reviewed Apr 11, 2022
View reviewed changes

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

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

Comment thread CMakeLists.txt Outdated
alexsavulescu
alexsavulescu reviewed Apr 11, 2022
View reviewed changes

@alexsavulescu alexsavulescu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

olupton added 6 commits April 11, 2022 15:14
@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 olupton force-pushed the olupton/version-macros branch from 2646a64 to 23ef82c Compare April 11, 2022 13:15
@olupton

olupton commented Apr 11, 2022

Copy link
Copy Markdown
Collaborator Author

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

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

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