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

Semantic Versioning and CLI --version support #51

Merged
merged 5 commits into from
Jul 6, 2017

Conversation

bemcdonnell
Copy link
Member

No description provided.

@bemcdonnell bemcdonnell added this to the v5.1.13 milestone Jun 22, 2017
@bemcdonnell
Copy link
Member Author

Addresses Issue #39 (Going to address #41 next)

@bemcdonnell bemcdonnell changed the title Semversion Semantic Versioning and CLI --version support Jun 22, 2017
@bemcdonnell bemcdonnell self-assigned this Jun 22, 2017
@bemcdonnell
Copy link
Member Author

One improvement - now we don't have to update the version in several places...

@dickinsonre
Copy link

@bemcdonnell as long as you are mentioning versioning - it would help as well to have the actual inp file have the engine version in the OPTIONS section. I often get past SWMM5 files and it is hard to figure out what version it matches. It would help parse the INP better - especially those predating SWMM 5.0.013

@bemcdonnell
Copy link
Member Author

@dickinsonre so are you suggesting we restrict an INP file to run with a specific version of SWMM?

@dickinsonre
Copy link

No, of course not but as i was reading all of the discussion of internal vs external parsing i remembered parsing issues with groundwater and a few other odd sections where a blank, a "*" or the number of tokens per line change the way the SWMM 5 engine builds the data structure. I hope this is clearer, but if you often get inp files from Korea or Vietnam (not to pick on those countries) it would be nice to know what version created the files.

@bemcdonnell
Copy link
Member Author

@dickinsonre: So essentially, which version of SWMM were we using when we "saved" the inp file?

src/consts.h Outdated
#define VERSION 51011 //(5.1.011)
// Update VERSION and SEMVERSION Simultaneously
#define VERSION 51013 // Eventually will be deprecated.
#define SEMVERSION "5.1.13.dev0" // Semantic Version
Copy link

@goanpeca goanpeca Jun 22, 2017

Choose a reason for hiding this comment

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

Actually I think it is more like

5.1.13dev0

We could store this as struct with major, minor, patch ?

struct VERSION_INFO {
   char  major[3];
   char  minor[3];
   char  patch[10];
};

Or something and generate VERSION and SEMVERSION from this with some helper funcs?

Copy link
Member Author

Choose a reason for hiding this comment

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

@goanpeca good suggestion - yes I can do that. Maybe patch should be longer since perhaps we might be adding some git vers numbers to it.

Choose a reason for hiding this comment

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

Sure, it can be, don't know how long though :-p

Copy link
Member Author

Choose a reason for hiding this comment

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

@goanpeca, I think splitting these up into major minor and patch is good but I'm not sure that containing this into a struct is necessary.

Copy link
Member Author

@bemcdonnell bemcdonnell Jun 26, 2017

Choose a reason for hiding this comment

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

I am happy to change the function to have three outputs:

int DLLEXPORT swmm_getSemVersion(char* major, char* minor, char* patch)

It leaves the user with more options.

Copy link
Member Author

Choose a reason for hiding this comment

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

The more I think about it, I think simply outputting the char array is the cleanest way.

@bemcdonnell
Copy link
Member Author

Here's the scoop:

You can now run:

swmm --help
swmm -h
swmm --version
swmm -v

Which gives:


STORMWATER MANAGEMENT MODEL (SWMM5) HELP

COMMANDS:
        --help (-h)       Help Docs
        --version (-v)    Build Version

RUNNING A SIMULATION:
        swmm5 <input file> <report file> <output file>

@bemcdonnell bemcdonnell requested a review from michaeltryby July 5, 2017 23:51
@goanpeca
Copy link

goanpeca commented Jul 5, 2017

Looks good :-)

Copy link

@michaeltryby michaeltryby left a comment

Choose a reason for hiding this comment

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

Since you are in here you might as well go ahead and separate main() from swmm5.c similar to what was done over in the EPANET repo. This way the dll and exe are two separate build targets and the exe can use the dll. Make sense?

src/swmm5.c Outdated
{
strcpy(major, SEMVERSION_MAJOR);
strcpy(minor, SEMVERSION_MINOR);
strcpy(patch, SEMVERSION_PATCH);

Choose a reason for hiding this comment

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

Use secure string copy strcpy_s() here instead.

src/swmm5.c Outdated
strcat(semver, SEMVERSION_MINOR);
strcat(semver, ".");
strcat(semver, SEMVERSION_PATCH);
}

Choose a reason for hiding this comment

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

This should be done in one line using a secure formatted print to string sprintf_s().


#define FMT03 " There are errors.\n"
#define FMT04 " There are warnings.\n"
#define FMT05 "\n"
#define FMT06 "\n o Retrieving project data"
#define FMT07 "\n o Writing output report"
#define FMT08 \
"\n EPA STORM WATER MANAGEMENT MODEL - VERSION 5.1 (Build 5.1.012)" //(5.1.012)

Choose a reason for hiding this comment

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

Are we sure about all the places FMT08 is being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure

@goanpeca
Copy link

goanpeca commented Jul 6, 2017

Since you are in here you might as well go ahead and separate main() from swmm5.c similar to what was done over in the EPANET repo. This way the dll and exe are two separate build targets and the exe can use the dll. Make sense?

Yes it makes sense 👍 !

@michaeltryby
Copy link

michaeltryby commented Jul 6, 2017

@goanpeca I see it fitting in here, but I think @bemcdonnell wants to handle separating main() in a separate pull.

@goanpeca
Copy link

goanpeca commented Jul 6, 2017

@goanpeca I see it fitting in here, but I think @bemcdonnell wants to handle separating main() in a separate pull.

Sure, it also works!

@bemcdonnell bemcdonnell merged commit 759a9ed into pyswmm:dev Jul 6, 2017
@bemcdonnell bemcdonnell deleted the semversion branch July 6, 2017 21:38
@bemcdonnell bemcdonnell modified the milestones: v5.1.13, v5.2.0.dev4 Oct 21, 2018
michaeltryby added a commit to SWMM-Project/swmm-solver that referenced this pull request Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants