add support for `--extra-source-urls` to fetch sources from additional URLs #4079

joeydumont · 2022-09-15T15:24:34Z

This PR adds a --sources-url CLI option. It is a comma separated list of URLs that EasyBuild will fetch sources from. It replaces the hard-coded EASYBUILD_SOURCES_URL, but keeps it as a default value. The --sources-url-priority option is used to determine if these URLs are to be queried first, or last, i.e. as a fallback.

Unfortunately, this breaks 3 tests related to downloading the file, and I'm unsure of exactly why. It seems that build_option('sources_url') might return None when the function is run through the test harness, rather than getting populated with the default value. I've tried adding a init_config(args=["--sources-url=https://sources.easybuild.io"]), but that didn't work either.

If the feature is of interest, could you give me a hand in fixing the tests?

EDIT: After some discussion on Slack, I was able to fix the tests. I also renamed the options to --extra-source-urls and --extra-source-urls-priority. I changed the commit mesage to be imperative.

easybuild/framework/easyblock.py

easybuild/tools/options.py

joeydumont · 2023-02-23T02:16:02Z

I see there are now merge conflicts for this. Let me know when you're ready for me to rebase, or if you prefer to handle that.

jfgrimm · 2023-02-23T16:00:21Z

I see there are now merge conflicts for this. Let me know when you're ready for me to rebase, or if you prefer to handle that.

go for it; if you have any issues/questions just ping me

Flamefire · 2024-05-22T08:24:00Z

Suggestion here:
--sources-url supports multiple URLs, doesn't it? How are they separated?

For EasyConfigs we have --robot-paths which accepts a list of paths like /path1:/path2 and works nicely mit the default paths if you append or prepend a colon: :/path1:/path2 appends it to the default, /path1:/path2: prepends it while the original replaces it.
How about replacing the priority option by something like this to have a uniform interface here?

joeydumont · 2024-07-15T19:11:25Z

As suggested, I made sure that this used the same add_flex logic that --robot-paths uses. However, I think URLs should be separated by | rather than a colon (:) or a comma (,), as both are valid in URLs. I added urllist to the list of options in ExtOption to allow for this separator to be used.

…l URLs. Add the --extra-source-urls CLI option, a | separated list of URLs that EasyBuild will fetch sources from. It replaces the hard-coded EASYBUILD_SOURCES_URL, but keeps it as a default value. Uses the add_flex logic to prepend, append, or insert into the default list. Co-authored-by: Alexander Grund <[email protected]>

Flamefire · 2024-07-16T07:04:51Z

easybuild/framework/easyblock.py

-                # add https://sources.easybuild.io as fallback source URL
-                source_urls.append(EASYBUILD_SOURCES_URL + '/' + os.path.join(name_letter, location))
+                # add extra-source-urls CLI as either a first check, or a fallback.
+                self.log.warning("[extra_source_urls] %s", build_option('extra_source_urls'))


Why are those warnings? I'd suggest to only log if there are any at all, as (at most) info and the individual urls as debug. And maybe only log the final value, not each intermediate

Thanks, I added those in for me to make sure the values were making their way in the expected places. I removed these logs completely, the debug logs already print

== 2024-07-16 20:58:08,626 generaloption.py:1604 DEBUG generate_cmd_line adding extra-source-urls value ('https://sources.easybuild.io/',) default found. Not adding to args.

Flamefire · 2024-07-16T07:05:26Z

easybuild/framework/easyblock.py

@@ -897,8 +895,13 @@ def obtain_file(self, filename, extension=False, urls=None, download_filename=No
                    source_urls = []
                source_urls.extend(self.cfg['source_urls'])

-                # add https://sources.easybuild.io as fallback source URL
-                source_urls.append(EASYBUILD_SOURCES_URL + '/' + os.path.join(name_letter, location))
+                # add extra-source-urls CLI as either a first check, or a fallback.


The comment seems outdated. I don't see an either-or in the code below. Also CLI should rather be "option" or "command line option" as CLI is usually the whole set of (all) CL options

It was, fixed.

Flamefire · 2024-07-16T07:08:45Z

easybuild/framework/easyblock.py

+                for url in build_option("extra_source_urls"):
+                    url += "/" + name_letter + "/" + location
+                    self.log.warning("[extra_source_urls] url is %s", url)
+                    source_urls.extend([url])


It's only a single element:

Suggested change

source_urls.extend([url])

source_urls.append(url)

Flamefire · 2024-07-16T07:10:12Z

easybuild/tools/options.py

@@ -407,6 +409,8 @@ def override_options(self):
                             None, 'store_true', False),
            'extra-modules': ("List of extra modules to load after setting up the build environment",
                              'strlist', 'extend', None),
+            "extra-source-urls": ("Specify URLs to fetch sources from in addition to those in the easyconfig",
+                                  "urltuple", "add_flex", DEFAULT_EXTRA_SOURCE_URLS, {'metavar': 'URL[%sURL]' % '|'}),


I'd rather not use string interpolation for a constant. Just makes loading EB (even) slower:

Suggested change

"urltuple", "add_flex", DEFAULT_EXTRA_SOURCE_URLS, {'metavar': 'URL[%sURL]' % '|'}),

"urltuple", "add_flex", DEFAULT_EXTRA_SOURCE_URLS, {'metavar': 'URL[|URL]'}),

Good point, done.

…sts.

Flamefire · 2024-08-13T06:50:16Z

easybuild/framework/easyblock.py

+                # Insert --extra-source-urls command line option to the
+                # urls to try to download from.


As mentioned earlier this is not only a CLI option, but can be from 2 other places. I suggest something like:

Suggested change

# Insert --extra-source-urls command line option to the

# urls to try to download from.

# Add additional URLs as configured

…ting whether specifying it here fixes the tests.

…NE so that init_config has the right default value. Should fix CI tests.

Flamefire

A final round of comments. All minor things, so it's just polishing this up. 👍

easybuild/base/generaloption.py

easybuild/framework/easyblock.py

easybuild/tools/config.py

easybuild/tools/options.py

easybuild/base/generaloption.py

easybuild/tools/options.py

easybuild/framework/easyblock.py

boegel · 2024-08-27T19:02:05Z

easybuild/tools/config.py

@@ -120,6 +120,7 @@
 DEFAULT_PR_TARGET_ACCOUNT = 'easybuilders'
 DEFAULT_PREFIX = os.path.join(os.path.expanduser('~'), ".local", "easybuild")
 DEFAULT_REPOSITORY = 'FileRepository'
+DEFAULT_EXTRA_SOURCE_URLS = ('https://sources.easybuild.io',)


Suggested change

DEFAULT_EXTRA_SOURCE_URLS = ('https://sources.easybuild.io',)

EASYBUILD_SOURCES_URL = 'https://sources.easybuild.io'

DEFAULT_EXTRA_SOURCE_URLS = (EASYBUILD_SOURCES_URL,)

…om config in easyblock leads to circular imports. Not sure how to stop houndbot from howling.

boegel · 2024-09-11T06:16:44Z

Good to go, thanks a lot for your patience with this one @joeydumont, I'm happy it's finally getting merged :)

joeydumont · 2024-09-11T11:47:03Z

Good to go, thanks a lot for your patience with this one @joeydumont, I'm happy it's finally getting merged :)

And thank you! I appreciate the feedback!

joeydumont force-pushed the source_url_cli_option branch 2 times, most recently from b5a6182 to de9ed77 Compare September 29, 2022 11:49

Flamefire reviewed Sep 29, 2022

View reviewed changes

easybuild/framework/easyblock.py Outdated Show resolved Hide resolved

Flamefire reviewed Sep 29, 2022

View reviewed changes

easybuild/tools/options.py Outdated Show resolved Hide resolved

joeydumont force-pushed the source_url_cli_option branch from 53f3f53 to 2641b81 Compare September 29, 2022 12:10

boegel added the enhancement label Oct 12, 2022

boegel added this to the 4.x milestone Oct 12, 2022

boegel changed the title ~~Adds a --sources-url CLI option to fetch sources from additional URLs.~~ add support for --sources-url to fetch sources from additional URLs Oct 12, 2022

jfgrimm reviewed Feb 22, 2023

View reviewed changes

easybuild/tools/options.py Outdated Show resolved Hide resolved

joeydumont force-pushed the source_url_cli_option branch from 2641b81 to 769d6fa Compare February 23, 2023 02:12

joeydumont force-pushed the source_url_cli_option branch from 769d6fa to f4d05b1 Compare May 21, 2024 12:53

joeydumont force-pushed the source_url_cli_option branch 2 times, most recently from ceb9c77 to dfd4d81 Compare July 15, 2024 19:08

joeydumont force-pushed the source_url_cli_option branch from e6cced2 to e9c8f7b Compare July 16, 2024 00:41

Flamefire suggested changes Jul 16, 2024

View reviewed changes

Addressed review comments.

18a9e29

boegel modified the milestones: 4.x, release after 4.9.2 Jul 31, 2024

boegel changed the title ~~add support for --sources-url to fetch sources from additional URLs~~ add support for --sources-url to fetch sources from additional URLs Jul 31, 2024

Handle the case build_option('extra_source_urls') is empty in unit te…

f4ec182

…sts.

Flamefire reviewed Aug 13, 2024

View reviewed changes

joeydumont added 2 commits August 13, 2024 10:43

extra-sources-url doesn't seem to be configured with init_config, tes…

281f9ba

…ting whether specifying it here fixes the tests.

Moved extra_source_urls to the proper location in BUILD_OPTIONS_CMDLI…

1f5542e

…NE so that init_config has the right default value. Should fix CI tests.

Flamefire suggested changes Aug 14, 2024

View reviewed changes

Addressed review comments.

d6a8d72

joeydumont added 2 commits August 14, 2024 21:18

Don't have an import alone on its line.

65410ff

Make houndbot happy.

6414ab0

Flamefire reviewed Aug 15, 2024

View reviewed changes

easybuild/tools/options.py Outdated Show resolved Hide resolved

Fix the order of imports.

6d98a2e

Flamefire approved these changes Aug 16, 2024

View reviewed changes

boegel requested changes Aug 27, 2024

View reviewed changes

boegel modified the milestones: 4.9.3, release after 4.9.3 Aug 28, 2024

joeydumont added 5 commits August 31, 2024 16:56

Address review comments.

dd86456

Chose the way that made houndbot happy.

d454bad

Need to revert to EASYBUILD_SOURCES_URL in easyblock, importing it fr…

0317785

…om config in easyblock leads to circular imports. Not sure how to stop houndbot from howling.

Remove unused import, define variable in two locations.

046e0a6

Don't repeat definitions.

f5fcdb4

boegel changed the title ~~add support for --sources-url to fetch sources from additional URLs~~ add support for --extra-source-urls to fetch sources from additional URLs Sep 11, 2024

boegel approved these changes Sep 11, 2024

View reviewed changes

boegel enabled auto-merge September 11, 2024 06:16

boegel merged commit 29c3e60 into easybuilders:develop Sep 11, 2024
37 checks passed

boegel modified the milestones: release after 4.9.3, 4.9.3 Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for `--extra-source-urls` to fetch sources from additional URLs #4079

add support for `--extra-source-urls` to fetch sources from additional URLs #4079

joeydumont commented Sep 15, 2022 •

edited

Loading

joeydumont commented Feb 23, 2023

jfgrimm commented Feb 23, 2023

Flamefire commented May 22, 2024 •

edited

Loading

joeydumont commented Jul 15, 2024

Flamefire Jul 16, 2024

joeydumont Jul 17, 2024

Flamefire Jul 16, 2024

joeydumont Jul 17, 2024

Flamefire Jul 16, 2024

joeydumont Jul 17, 2024

Flamefire Jul 16, 2024

joeydumont Jul 17, 2024

Flamefire Aug 13, 2024

Flamefire left a comment

boegel Aug 27, 2024

boegel commented Sep 11, 2024

joeydumont commented Sep 11, 2024

	"urltuple", "add_flex", DEFAULT_EXTRA_SOURCE_URLS, {'metavar': 'URL[%sURL]' % '\|'}),
	"urltuple", "add_flex", DEFAULT_EXTRA_SOURCE_URLS, {'metavar': 'URL[\|URL]'}),

		# Insert --extra-source-urls command line option to the
		# urls to try to download from.

	# Insert --extra-source-urls command line option to the
	# urls to try to download from.
	# Add additional URLs as configured

	DEFAULT_EXTRA_SOURCE_URLS = ('https://sources.easybuild.io',)
	EASYBUILD_SOURCES_URL = 'https://sources.easybuild.io'
	DEFAULT_EXTRA_SOURCE_URLS = (EASYBUILD_SOURCES_URL,)

add support for --extra-source-urls to fetch sources from additional URLs #4079

add support for --extra-source-urls to fetch sources from additional URLs #4079

Conversation

joeydumont commented Sep 15, 2022 • edited Loading

joeydumont commented Feb 23, 2023

jfgrimm commented Feb 23, 2023

Flamefire commented May 22, 2024 • edited Loading

joeydumont commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flamefire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Sep 11, 2024

joeydumont commented Sep 11, 2024

add support for `--extra-source-urls` to fetch sources from additional URLs #4079

add support for `--extra-source-urls` to fetch sources from additional URLs #4079

joeydumont commented Sep 15, 2022 •

edited

Loading

Flamefire commented May 22, 2024 •

edited

Loading