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

Introduce Go Modules to tooling #1901

Merged
merged 4 commits into from
Nov 27, 2023
Merged

Introduce Go Modules to tooling #1901

merged 4 commits into from
Nov 27, 2023

Conversation

aaomidi
Copy link
Contributor

@aaomidi aaomidi commented Nov 20, 2023

Solves #1897 - also spoken of here: #1325 (comment).

I initially added this with go workspaces, but turns out those aren't actually meant to be pushed (even though, in this specific scenario, they made the commands a little bit cleaner).

I've made sure that none of the existing workflows fail with this change.

I've moved the go tools outside of tools. This wasn't absolutely necessary but I didn't want the bash scripts and the go module to live in the same directory -- I think it would've added additional long term cost to keep them in the same directory.

I've updated all the github actions workflows to keep working with the new layout.

make test:

~~ redacted
PASS: libpsl_icu_load_dafsa_fuzzer
PASS: libpsl_icu_fuzzer
PASS: libpsl_icu_load_fuzzer
============================================================================
Testsuite summary for libpsl 0.21.2
============================================================================
# TOTAL: 3
# PASS:  3
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

~~ redacted

PASS: test-is-public-builtin
PASS: test-is-public
PASS: test-registrable-domain
PASS: test-is-cookie-domain-acceptable
PASS: test-is-public-all
============================================================================
Testsuite summary for libpsl 0.21.2
============================================================================
# TOTAL: 5
# PASS:  5
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

$ ./tools/patchnewgtlds 
++ command -v go
+ '[' -x /usr/local/go/bin/go ']'
++ realpath ./tools/patchnewgtlds
+ SCRIPT=/workspaces/list/tools/patchnewgtlds
++ dirname /workspaces/list/tools/patchnewgtlds
+ BASEDIR=/workspaces/list/tools
+ go run -C /workspaces/list/tools/../newgtlds . -overwrite -psl-dat-file=/workspaces/list/tools/../public_suffix_list.dat

@aaomidi aaomidi changed the title Introduce Go Modules Introduce Go Modules to tooling Nov 20, 2023
@aaomidi aaomidi marked this pull request as ready for review November 20, 2023 22:16
@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 20, 2023

@cpu @weppos I've introduced the new go package here.

@weppos
Copy link
Member

weppos commented Nov 21, 2023

Thanks for your help @aaomidi. I will need to ask you a change. Please move the newgtlds package into the tools folder. As this repository is already a mix of many different things, I would prefer to reduce the creation of new folders inside the root.

@weppos weppos self-assigned this Nov 21, 2023
@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 21, 2023

@weppos thanks for the feedback! Is this what you intended?

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

tools/go.mod Outdated Show resolved Hide resolved
.github/workflows/tld-update.yml Outdated Show resolved Hide resolved
@weppos weppos self-requested a review November 23, 2023 13:32
@mozfreddyb mozfreddyb added the non .dat Change or Coding Review Alteration to code/automation or publicsuffix.org site label Nov 27, 2023
@weppos weppos merged commit 2d1427a into publicsuffix:master Nov 27, 2023
1 check passed
@weppos
Copy link
Member

weppos commented Nov 27, 2023

Thanks @aaomidi. Also thanks @cpu for the co-review.

@weppos
Copy link
Member

weppos commented Nov 27, 2023

FWIW, I am unable to run the tests locally with that command.

go test -v -C ./tools .
go: cannot find main module, but found .git/config in /Users/.../Code/publicsuffix-list
	to create a module there, run:
	go mod init

I am instead if I run it from the tools folder:

cd tools/
go test -v ./...
=== RUN   TestEntryNormalize
=== RUN   TestEntryNormalize/already_normalized
=== RUN   TestEntryNormalize/extra_whitespace
=== RUN   TestEntryNormalize/no_explicit_uLabel
--- PASS: TestEntryNormalize (0.00s)
    --- PASS: TestEntryNormalize/already_normalized (0.00s)
    --- PASS: TestEntryNormalize/extra_whitespace (0.00s)
    --- PASS: TestEntryNormalize/no_explicit_uLabel (0.00s)
=== RUN   TestEntryComment
=== RUN   TestEntryComment/Full_entry
=== RUN   TestEntryComment/Entry_without_operator
=== RUN   TestEntryComment/Entry_with_non-empty_operator
--- PASS: TestEntryComment (0.00s)
    --- PASS: TestEntryComment/Full_entry (0.00s)
    --- PASS: TestEntryComment/Entry_without_operator (0.00s)
    --- PASS: TestEntryComment/Entry_with_non-empty_operator (0.00s)
=== RUN   TestGetData
--- PASS: TestGetData (0.00s)
=== RUN   TestGetPSLEntries
--- PASS: TestGetPSLEntries (0.00s)
=== RUN   TestGetPSLEntriesEmptyResults
--- PASS: TestGetPSLEntriesEmptyResults (0.00s)
=== RUN   TestGetPSLEntriesEmptyFilteredResults
--- PASS: TestGetPSLEntriesEmptyFilteredResults (0.00s)
=== RUN   TestRenderData
--- PASS: TestRenderData (0.00s)
=== RUN   TestErrInvertedSpan
--- PASS: TestErrInvertedSpan (0.00s)
=== RUN   TestGTLDDatSpanValidate
=== RUN   TestGTLDDatSpanValidate/no_header
=== RUN   TestGTLDDatSpanValidate/no_footer
=== RUN   TestGTLDDatSpanValidate/inverted
=== RUN   TestGTLDDatSpanValidate/valid
--- PASS: TestGTLDDatSpanValidate (0.00s)
    --- PASS: TestGTLDDatSpanValidate/no_header (0.00s)
    --- PASS: TestGTLDDatSpanValidate/no_footer (0.00s)
    --- PASS: TestGTLDDatSpanValidate/inverted (0.00s)
    --- PASS: TestGTLDDatSpanValidate/valid (0.00s)
=== RUN   TestErrSpanOutOfBounds
--- PASS: TestErrSpanOutOfBounds (0.00s)
=== RUN   TestDatFileValidate
=== RUN   TestDatFileValidate/bad_gTLD_span
=== RUN   TestDatFileValidate/out_of_bounds_span
=== RUN   TestDatFileValidate/valid
--- PASS: TestDatFileValidate (0.00s)
    --- PASS: TestDatFileValidate/bad_gTLD_span (0.00s)
    --- PASS: TestDatFileValidate/out_of_bounds_span (0.00s)
    --- PASS: TestDatFileValidate/valid (0.00s)
=== RUN   TestGetGTLDLines
--- PASS: TestGetGTLDLines (0.00s)
=== RUN   TestReplaceGTLDContent
--- PASS: TestReplaceGTLDContent (0.00s)
=== RUN   TestDatFileString
--- PASS: TestDatFileString (0.00s)
=== RUN   TestReadDatFile
=== RUN   TestReadDatFile/no_such_file
=== RUN   TestReadDatFile/no_header
=== RUN   TestReadDatFile/no_footer
=== RUN   TestReadDatFile/multiple_headers
=== RUN   TestReadDatFile/inverted_header/footer
=== RUN   TestReadDatFile/valid
--- PASS: TestReadDatFile (0.00s)
    --- PASS: TestReadDatFile/no_such_file (0.00s)
    --- PASS: TestReadDatFile/no_header (0.00s)
    --- PASS: TestReadDatFile/no_footer (0.00s)
    --- PASS: TestReadDatFile/multiple_headers (0.00s)
    --- PASS: TestReadDatFile/inverted_header/footer (0.00s)
    --- PASS: TestReadDatFile/valid (0.00s)
=== RUN   TestProcess
=== RUN   TestProcess/bad_span
=== RUN   TestProcess/span_too_small
=== RUN   TestProcess/no_change_in_data
=== RUN   TestProcess/change_in_data
--- PASS: TestProcess (0.00s)
    --- PASS: TestProcess/bad_span (0.00s)
    --- PASS: TestProcess/span_too_small (0.00s)
    --- PASS: TestProcess/no_change_in_data (0.00s)
    --- PASS: TestProcess/change_in_data (0.00s)
PASS
ok  	github.com/publicsuffix/list/tools	0.355s

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 27, 2023

FWIW, I am unable to run the tests locally with that command.

go test -v -C ./tools .
go: cannot find main module, but found .git/config in /Users/.../Code/publicsuffix-list
	to create a module there, run:
	go mod init

It seems that Go requires the -C to be first:

go test -C ./tools -v .

Worked fine for me.

@cpu
Copy link
Contributor

cpu commented Nov 27, 2023

It seems that Go requires the -C to be first:

Sorry that my suggestion ended up causing headaches. Does that mean the tld-update.yml needs to be adjusted?

- name: Run unit tests
run: go test -v -C ./tools .

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 27, 2023

Ah yeah, seems like it. I'll create a quick PR to fix that.

And no problem! I honestly wasn't expecting Go to care about flag order like this.

@weppos
Copy link
Member

weppos commented Nov 28, 2023

It seems that Go requires the -C to be first:

It still doesn't work in my system. I'll try to figure out why.

go test -C ./tools -v .
go: cannot find main module, but found .git/config in /Users/.../Code/publicsuffix-list
	to create a module there, run:
	go mod init

Sorry that my suggestion ended up causing headaches. Does that mean the tld-update.yml needs to be adjusted?

Don't worry. Actually, it seems to work on the CI. For some reason, my local env doesn't like it.

@weppos
Copy link
Member

weppos commented Nov 28, 2023

Don't worry. Actually, it seems to work on the CI. For some reason, my local env doesn't like it.

Actually, online as well is broken. 😅
https://github.com/publicsuffix/list/actions/runs/7006982080/job/19060050437

I'll look into the PR @aaomidi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non .dat Change or Coding Review Alteration to code/automation or publicsuffix.org site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants