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

Jump to bottom

builder: File paths are not relative, bug fixes #114

Merged
merged 14 commits into from Mar 26, 2022
Merged

builder: File paths are not relative, bug fixes #114

merged 14 commits into from Mar 26, 2022

Conversation

CatalinStratu
Copy link
Contributor

After my changes:
image

Old:
image

Also, I've added a comment containing the command that generates the documentation to make it easier to test.
Have a nice day!!

seabass-labrax
seabass-labrax approved these changes Mar 14, 2022
View reviewed changes
Copy link

@seabass-labrax seabass-labrax left a comment

Choose a reason for hiding this comment

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

Great work, @CatalinStratu!

builder/builder2v1/build_package.go Show resolved Hide resolved
examples/3-build/example_build.go
@@ -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
Copy link

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?

@swinslow
Copy link
Member

Hi @CatalinStratu, thanks for this! It looks like it's in the right direction, I have a couple of comments below:

  1. Just to clarify, the FileName file paths should be relative to the root of the package being scanned, rather than relative to the place where the program is running. For example, if we have the following at tools-golang/testdata/project1/:
testdata
├── project1
│   ├── emptyfile.testdata.txt
│   ├── file1.testdata.txt
│   ├── file3.testdata.txt
│   ├── folder1
│   │   └── file4.testdata.txt
│   ├── lastfile.testdata.txt
. . .

and we're running a program which is at tools-golang/examples/3-build/, then when we're describing the Files in the project1 Package, the FileNames should be:

./emptyfile.testdata.txt
./file1.testdata.txt
./file3.testdata.txt
./folder1/file4.testdata.txt
. . .

rather than ./../../testdata/project1/emptyfile.testdata.txt, etc. Does that make sense?

  1. I agree with @seabass-labrax above, it's a great addition to have the comment in the examples/ source code for each example program explaining how to run it. I'm okay with you leaving in this PR the one you added for example 3, but if so then could you also submit a separate issue so that we remember to add something similar for the others? Thanks!

@swinslow swinslow mentioned this pull request Mar 14, 2022
Closed
@CatalinStratu
Copy link
Contributor Author

@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.

@CatalinStratu
builder: File paths are not relative, bug fixes
bca2ddb 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
builder: File paths are not relative, bug fix
64add55 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
builder: File paths are not relative, bug fix 2.1 version
8feaee9 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
bug fixes
afc5d64 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
file patch fixes
d87b065 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
file patch fixes
673cb49 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
test fixes
afab6dd 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
test fixes
d39772c 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
test fixes
13f9275 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
Path fixes
1e3c66b 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
Path fixes
1eaa6c8 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
Fixes for Linux OS
18d9036 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
Fixes signoff
bc89f8d 
Signed-off-by: Catalin <catalinstratu45@gmail.com>
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@CatalinStratu
Fixes patch for WINDOWS OS
f7ea935 
Signed-off-by: Catalin Stratu <catalinstratu45@gmail.com>
@swinslow
Copy link
Member

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.

@CatalinStratu
Copy link
Contributor Author

@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

@CatalinStratu CatalinStratu mentioned this pull request Mar 16, 2022
Closed
@CatalinStratu
Copy link
Contributor Author

Hi @swinslow, what do you think of this PR??

@swinslow
Copy link
Member

Hi @CatalinStratu, apologies for the delayed response. I am reviewing now and will merge or else circle back shortly with any further thoughts.

@swinslow
Copy link
Member

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!

@swinslow swinslow merged commit bea3d05 into spdx:main Mar 26, 2022
@swinslow swinslow added this to the 0.3.0 milestone Mar 27, 2022
@swinslow swinslow added the bug Something isn't working label Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seabass-labrax seabass-labrax seabass-labrax approved these changes

Assignees

@CatalinStratu CatalinStratu

Labels
bug Something isn't working
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@CatalinStratu @swinslow @seabass-labrax