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

Added new ntpstat port v0.2 #13371

Merged
merged 1 commit into from Dec 16, 2021
Merged

Added new ntpstat port v0.2 #13371

merged 1 commit into from Dec 16, 2021

Conversation

RobK88
Copy link
Contributor

@RobK88 RobK88 commented Dec 15, 2021

Description

ntpstat: New port, version 0.2

Closes: https://trac.macports.org/ticket/64142

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS x.y
Xcode x.y

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

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

thanks for the submission @RobK88 - a few comments after a cursory read through

sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Show resolved Hide resolved
Copy link
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

The commit message does not conform to our guidelines (despite the checkbox stating that it does being checked). A conforming commit message could be:

ntpstat: New port, version 0.2

Closes: https://trac.macports.org/ticket/64142

sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
sysutils/ntpstat/Portfile Outdated Show resolved Hide resolved
@ryandesign
Copy link
Contributor

Sorry about duplicate suggestions; looks like Renee and I were commenting at the same time.

@RobK88
Copy link
Contributor Author

RobK88 commented Dec 15, 2021

Thanks Ryan and Renee for all your comments. After running sudo port -vst install ntpstat, I saw the issue with not setting up the compiler and the compiler flags properly. This is the first time I have used the GitHib PortGroup.(P.S. It is too bad the online Macport Guide did not mention testing the install using trace mode. I only saw this requirement after I submitted my PR request).

I agree that the simple solution is to add "PortGroup Makefile 1.0" and remove "use_configure no".

Do you want me to do this and submit another Pull Request?

P.S. I also need to reverse my commit suggestion. The new checksums suggested did not work. I needed to use these instead:

                rmd160  7481a76c3ac765513113b8db4e0eee3479a65413 \
                sha256  f1430f105902f20b4c7c3abdd3cbd69162b0e9275e6967bb620d546572bed8de \
                size    3962

@RobK88
Copy link
Contributor Author

RobK88 commented Dec 15, 2021

I have an new updated portfile for ntpstat incorporating your suggestions. I can install with trace mode ON.
Should I simply close this PR request and create a new PR request? Maybe that is easier.

Attached is my updated portfile. (I add to add .txt before I could upload it)/
Portfile.txt

@reneeotten
Copy link
Contributor

Should I simply close this PR request and create a new PR request? Maybe that is easier.

Next time you make changes just commit them with git commit --amend that will just change the one commit you have and you can force-push to this PR. Now you can just commit the new Portfile you have and then you "rebase" your commits. Do git rebase -i HEAD~#, where # is the number of commits; so probably 5 by the time you've comitted your latest Portfile. That will open up a text editor and you can change "pick" to "fixup" for all except the first commit; then change your commit message accordingly and push to this branch.

There are many website with manuals on how to use git in the context of PRs and such that explain all of this in more detail.

@RobK88
Copy link
Contributor Author

RobK88 commented Dec 15, 2021

git commit --amend

Thanks Renee. I was trying to do this just using a forked repo on Github.com. I don''t think you can do a git commit --amend from the GitHub.com website.

In future, I will just clone the macports repo to my Mac, create a branch, edit files, rebase, submit a pull request -- all using the command line. I can then do a git commit --amend as you recommended for additional changes. etc.

P.S. I am new to DISTRIBUTED CVS. In fact when I started to program in the 70s on mainframes using punch cards, CVS type systems did not even exist.

What confused me in the Macports Guide were the terms git and Github. In my view, the documentation should make it clear that the term git DOES NOT include a repo on github.com. (I am sure that experienced developers who use git and Github would not confuse the two. But that is not the case for git newbies). And unfortunately, the capabilities of GitHub.com is more limited than a local git repo (as I just discovered).

One needs to clone the Macports GitHub repo to a local git repo on their own Mac, make changes, commit, rebase etc and then make a Pull Request (PR) from the local git repo to the Macport Github site. And optionally one can push one's changes from the local git repo to one's personal GitHub.com repo (which was presumably forked or cloned from the Macports GitHub repo).

The Macports Guide does not mention using git commit --amend and a force push for future small changes but I agree that is a wise approach to minimize the number of commits that you need to deal with.

@RobK88
Copy link
Contributor Author

RobK88 commented Dec 16, 2021

Sorry for the last commit. Next time, I will just clone the macports repo to a local repo on my Mac and use got command on the command line so I can do a commit --amend and a force push. Just using my personal repo on Githib.com does not allow me to do that.

In any event the last commit should address all the issues raised. The port works and installs even when using trace mode. Thanks for all your help.

@reneeotten
Copy link
Contributor

@RobK88 I made the last few changes I requested myself and cleaned-up the commit history; please take a look and once the CI finished correctly I think this should be ready to merge. Thanks again for the submission!

@RobK88
Copy link
Contributor Author

RobK88 commented Dec 16, 2021

Thanks Renee. I have no problems with any of your suggested changes.
I just hope it will now build okay on 10.15 and MacOS 11.
Thanks again for all your help. And thanks for cleaning up the commit history. I know I made a mess of that.

P.S. As for the line CC = gcc in the Makefile. I was told on my first port submission (codegroup) that it does not make a difference if one uses CC = gcc or CC ?= gcc in the Makefile as far as Macports is concerned. Apparently, Macports will override the CC variable whenever needed!

It is not difficult to override an environment variable in a Makefile even one that has been assigned using = or :=. One just needs to pass the the new value for the environment variable with the make command. e.g. make CC=gcc. (But please note: export CC=gcc; make will NOT override the CC environment variable in a Makefile that has been assigned using = or :=. You must use make CC=gcc). See ​https://stackoverflow.com/questions/18007326/how-to-change-default-values-of-variables-like-cc-in-makefile

@reneeotten reneeotten merged commit 10d23ca into macports:master Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants