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

MinGW, CMake and project layout #15

Closed
wants to merge 10 commits into from
Closed

Conversation

squimrel
Copy link
Contributor

@squimrel squimrel commented Jul 4, 2017

I'm pretty unsure about the MinGW package name in other words this needs review.

@squimrel squimrel changed the title WIP: MinGW, CMake and project layout. MinGW, CMake and project layout Jul 5, 2017
@squimrel squimrel force-pushed the mingw branch 4 times, most recently from df4e7d4 to c624953 Compare July 5, 2017 23:57
@bcl
Copy link
Collaborator

bcl commented Jul 10, 2017

I don't have any interesting in making this MinGW compatible. The changes are too extensive for something that won't be used (tested) by the majority of the users so it'll end up rotting.

@squimrel
Copy link
Contributor Author

squimrel commented Jul 10, 2017

I need this for the Fedora Media Writer because it should work on Windows. I don't know who else is a user of isomd5sum apart from people using the command-line tool I can't find any projects that use it.

I could have also changed the Makefile (less changes) but doing it properly using CMake seemed cleaner in the long run.
Also it's supposed to make packaging easier and especially take away the work from you to package for mingw that's why there're so many changes.

@squimrel squimrel force-pushed the mingw branch 2 times, most recently from 2b7b789 to 0883be9 Compare July 10, 2017 21:16
@bcl
Copy link
Collaborator

bcl commented Jul 10, 2017

Why does media writer need it? Is it going to be doing a pre-check of the embedded checksums instead of (or in addition to) the SHA of the iso?

@squimrel
Copy link
Contributor Author

squimrel commented Jul 10, 2017

It does an SHA-256 check after download.
Then it might modify the iso image and therefore re-implant the checksum.
To ensure that nothing went wrong when writing to disk it's doing an isomd5sum check and then after it modified the partition table it updates the isomd5sum.
So it needs both check and implant.

@squimrel squimrel force-pushed the mingw branch 2 times, most recently from 4a8bd89 to c38053d Compare July 11, 2017 02:13
@bcl
Copy link
Collaborator

bcl commented Jul 11, 2017

Then it might modify the iso image and therefore re-implant the checksum.

Ah, ok! Thanks for the explanation.

Copy link
Collaborator

@bcl bcl left a comment

Choose a reason for hiding this comment

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

I think my biggest concern with this now is the .spec.in stuff. The spec file shouldn't really be in the upstream project, and it certainly shouldn't be running fedpkg commands from there.

We should also call this 1.3.0 instead of 1.2.2 (I'm going to do a build of current master as 1.2.2).

@@ -24,8 +24,8 @@
#include <popt.h>
#include <termios.h>

#include "md5.h"
#include "libcheckisomd5.h"
#include "./include/libcheckisomd5.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be needed if you can pass the include path to it from cmake

Copy link
Contributor Author

@squimrel squimrel Aug 25, 2017

Choose a reason for hiding this comment

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

Well, do I want to tell CMake to search in ./include/? I simply like being explicit about things.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, telling cmake about ./include/ is the right way to do it.

@squimrel
Copy link
Contributor Author

squimrel commented Aug 26, 2017

@bcl Okay I removed all the packaging stuff from the PR. I'll leave that to the packager then.

@squimrel squimrel force-pushed the mingw branch 2 times, most recently from 2d3f91a to f2faf89 Compare August 26, 2017 00:28
@bcl
Copy link
Collaborator

bcl commented Sep 15, 2017

I think there's a couple other things to add/figure out:

  • version number -- you have it pulling from the git tag, but that doesn't help if someone has a bare archive of the code. It used to be in the Makefile and tagged from that.
  • Creating an archive -- There should be a make target for this like there used to be: https://github.com/rhinstaller/isomd5sum/blob/master/Makefile#L72
  • Add ./build/ to .gitignore
  • Add a section to the README.md on how to create a new release.

Thanks!

@squimrel
Copy link
Contributor Author

Sorry for the late response. I addressed your last comment. But AFAIK you can not add custom rules to the Makefile that cmake creates so instead I added a script which creates that archive at scripts/create-archive.

@bcl
Copy link
Collaborator

bcl commented Jan 4, 2018

Thanks for the updated patches, but I'm going to have to nak these changes. I've spend the afternoon trying to get it working with an updated fedora spec file and I keep hitting problems. It just introduces too much change for my taste.

@bcl bcl closed this Jan 4, 2018
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.

2 participants