-
Notifications
You must be signed in to change notification settings - Fork 61
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
builder: File paths are not relative, bug fixes #114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @CatalinStratu!
@@ -5,6 +5,7 @@ | |||
// This example demonstrates building an 'empty' SPDX document in memory that | |||
// corresponds to a given directory's contents, including all files with their | |||
// hashes and the package's verification code, and saving the document to disk. | |||
// Run project: go run example_build.go project2 ../../testdata/project2 test.spdx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good addition; maybe it would be worth leaving it out of this pull request, and then creating another to add such a line to all of the examples?
Hi @CatalinStratu, thanks for this! It looks like it's in the right direction, I have a couple of comments below:
and we're running a program which is at
rather than
|
@swinslow Hi, "spdx / tools-golang" behaves differently on Windows than on Linux. I made some changes and for Linux now it works perfectly, I'm making some changes so that it works well on Windows as well. |
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin <[email protected]> Signed-off-by: Catalin Stratu <[email protected]>
Signed-off-by: Catalin Stratu <[email protected]>
Thank you @CatalinStratu! I'll review the updates later today and will merge if it all looks good to go, or will circle back with any follow-up comments or questions. |
@swinslow I added 'if osType == "windows"' because in Linux the folder looks good, in Windows it adds "/../../". After this PR I created a new PR explaining how to run the examples |
Hi @swinslow, what do you think of this PR?? |
Hi @CatalinStratu, apologies for the delayed response. I am reviewing now and will merge or else circle back shortly with any further thoughts. |
Although I haven't yet been able to test this myself on Windows, I'm not seeing any issues based on my tests on Linux. So I'm going to go ahead and merge this. @CatalinStratu thanks for your help and patience! |
After my changes:
![image](https://user-images.githubusercontent.com/28156781/158187077-183e4768-2d17-49d5-91f1-c2291cd0964a.png)
Old:
![image](https://user-images.githubusercontent.com/28156781/158187282-7eee2d53-597b-487f-85bb-e68089d7202c.png)
Also, I've added a comment containing the command that generates the documentation to make it easier to test.
Have a nice day!!